[PATCH v2 0/7] Copy hashmap to tools/perf/util, use in perf expr

[PATCH v2 0/7] Copy hashmap to tools/perf/util, use in perf expr

[Ian Rogers]

Perf's expr code currently builds an array of strings then removes
duplicates. The array is larger than necessary and has recently been
increased in size. When this was done it was commented that a hashmap
would be preferable.

libbpf has a hashmap but libbpf isn't currently required to build
perf. To satisfy various concerns this change copies libbpf's hashmap
into tools/perf/util, it then adds a check in perf that the two are in
sync.

Andrii's patch to hashmap from bpf-next is brought into this set to
fix issues with hashmap__clear.

Two minor changes to libbpf's hashmap are made that remove an unused
dependency and fix a compiler warning.

Two perf test changes are also brought in as they need refactoring to
account for the expr API change and it is expected they will land
ahead of this.
https://lore.kernel.org/lkml/20200513062236.854-2-irogers@google.com/

Tested with 'perf test' and 'make -C tools/perf build-test'.

The hashmap change was originally part of an RFC:
https://lore.kernel.org/lkml/20200508053629.210324-1-irogers@google.com/

v2. moves hashmap into tools/perf/util rather than libapi, to allow
hashmap's libbpf symbols to be visible when built statically for
testing.

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

Ian Rogers (6):
  libbpf hashmap: Remove unused #include
  libbpf hashmap: Fix signedness warnings
  tools lib/api: Copy libbpf hashmap to tools/perf/util
  perf test: Provide a subtest callback to ask for the reason for
    skipping a subtest
  perf test: Improve pmu event metric testing
  perf expr: Migrate expr ids table to a hashmap

 tools/lib/bpf/hashmap.c         |  10 +-
 tools/lib/bpf/hashmap.h         |   1 -
 tools/perf/check-headers.sh     |   4 +
 tools/perf/tests/builtin-test.c |  18 ++-
 tools/perf/tests/expr.c         |  40 +++---
 tools/perf/tests/pmu-events.c   | 169 ++++++++++++++++++++++-
 tools/perf/tests/tests.h        |   4 +
 tools/perf/util/Build           |   4 +
 tools/perf/util/expr.c          | 129 +++++++++--------
 tools/perf/util/expr.h          |  26 ++--
 tools/perf/util/expr.y          |  22 +--
 tools/perf/util/hashmap.c       | 238 ++++++++++++++++++++++++++++++++
 tools/perf/util/hashmap.h       | 177 ++++++++++++++++++++++++
 tools/perf/util/metricgroup.c   |  87 ++++++------
 tools/perf/util/stat-shadow.c   |  49 ++++---
 15 files changed, 798 insertions(+), 180 deletions(-)
 create mode 100644 tools/perf/util/hashmap.c
 create mode 100644 tools/perf/util/hashmap.h

-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH v2 1/7] libbpf: Fix memory leak and possible double-free in hashmap__clear

[Ian Rogers]

From: Andrii Nakryiko <andriin@fb.com>

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

Running test_progs-asan flavor clearly showed this problem.

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

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

[PATCH v2 2/7] libbpf hashmap: Remove unused #include

[Ian Rogers]

Remove #include of libbpf_internal.h that is unused.
Discussed in this thread:
https://lore.kernel.org/lkml/CAEf4BzZRmiEds_8R8g4vaAeWvJzPb4xYLnpF0X2VNY8oTzkphQ@mail.gmail.com/

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

diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h
index bae8879cdf58..e823b35e7371 100644
--- a/tools/lib/bpf/hashmap.h
+++ b/tools/lib/bpf/hashmap.h
@@ -15,7 +15,6 @@
 #else
 #include <bits/reg.h>
 #endif
-#include "libbpf_internal.h"
 
 static inline size_t hash_bits(size_t h, int bits)
 {
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH v2 3/7] libbpf hashmap: Fix signedness warnings

[Ian Rogers]

Fixes the following warnings:

hashmap.c: In function ‘hashmap__clear’:
hashmap.h:150:20: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
  150 |  for (bkt = 0; bkt < map->cap; bkt++)        \

hashmap.c: In function ‘hashmap_grow’:
hashmap.h:150:20: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
  150 |  for (bkt = 0; bkt < map->cap; bkt++)        \

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

diff --git a/tools/lib/bpf/hashmap.c b/tools/lib/bpf/hashmap.c
index cffb96202e0d..a405dad068f5 100644
--- a/tools/lib/bpf/hashmap.c
+++ b/tools/lib/bpf/hashmap.c
@@ -60,7 +60,7 @@ struct hashmap *hashmap__new(hashmap_hash_fn hash_fn,
 void hashmap__clear(struct hashmap *map)
 {
 	struct hashmap_entry *cur, *tmp;
-	int bkt;
+	size_t bkt;
 
 	hashmap__for_each_entry_safe(map, cur, tmp, bkt) {
 		free(cur);
@@ -100,8 +100,7 @@ static int hashmap_grow(struct hashmap *map)
 	struct hashmap_entry **new_buckets;
 	struct hashmap_entry *cur, *tmp;
 	size_t new_cap_bits, new_cap;
-	size_t h;
-	int bkt;
+	size_t h, bkt;
 
 	new_cap_bits = map->cap_bits + 1;
 	if (new_cap_bits < HASHMAP_MIN_CAP_BITS)
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH v2 4/7] tools lib/api: Copy libbpf hashmap to tools/perf/util

[Ian Rogers]

Allow use of hashmap in perf. Modify perf's check-headers.sh script to
check that the files are kept in sync, in the same way kernel headers are
checked. This will warn if they are out of sync at the start of a perf
build.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/check-headers.sh |   4 +
 tools/perf/util/Build       |   4 +
 tools/perf/util/hashmap.c   | 238 ++++++++++++++++++++++++++++++++++++
 tools/perf/util/hashmap.h   | 177 +++++++++++++++++++++++++++
 4 files changed, 423 insertions(+)
 create mode 100644 tools/perf/util/hashmap.c
 create mode 100644 tools/perf/util/hashmap.h

diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index cf147db4e5ca..94c2bc22c2bb 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -128,4 +128,8 @@ check arch/x86/lib/insn.c             '-I "^#include [\"<]\(../include/\)*asm/in
 # diff non-symmetric files
 check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl
 
+# check duplicated library files
+check_2 tools/perf/util/hashmap.h tools/lib/bpf/hashmap.h
+check_2 tools/perf/util/hashmap.c tools/lib/bpf/hashmap.c
+
 cd tools/perf
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index ca07a162d602..880fcdd1ab11 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -136,6 +136,10 @@ perf-$(CONFIG_LIBELF) += symbol-elf.o
 perf-$(CONFIG_LIBELF) += probe-file.o
 perf-$(CONFIG_LIBELF) += probe-event.o
 
+ifndef CONFIG_LIBBPF
+perf-y += hashmap.o
+endif
+
 ifndef CONFIG_LIBELF
 perf-y += symbol-minimal.o
 endif
diff --git a/tools/perf/util/hashmap.c b/tools/perf/util/hashmap.c
new file mode 100644
index 000000000000..a405dad068f5
--- /dev/null
+++ b/tools/perf/util/hashmap.c
@@ -0,0 +1,238 @@
+// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+
+/*
+ * Generic non-thread safe hash map implementation.
+ *
+ * Copyright (c) 2019 Facebook
+ */
+#include <stdint.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <errno.h>
+#include <linux/err.h>
+#include "hashmap.h"
+
+/* make sure libbpf doesn't use kernel-only integer typedefs */
+#pragma GCC poison u8 u16 u32 u64 s8 s16 s32 s64
+
+/* start with 4 buckets */
+#define HASHMAP_MIN_CAP_BITS 2
+
+static void hashmap_add_entry(struct hashmap_entry **pprev,
+			      struct hashmap_entry *entry)
+{
+	entry->next = *pprev;
+	*pprev = entry;
+}
+
+static void hashmap_del_entry(struct hashmap_entry **pprev,
+			      struct hashmap_entry *entry)
+{
+	*pprev = entry->next;
+	entry->next = NULL;
+}
+
+void hashmap__init(struct hashmap *map, hashmap_hash_fn hash_fn,
+		   hashmap_equal_fn equal_fn, void *ctx)
+{
+	map->hash_fn = hash_fn;
+	map->equal_fn = equal_fn;
+	map->ctx = ctx;
+
+	map->buckets = NULL;
+	map->cap = 0;
+	map->cap_bits = 0;
+	map->sz = 0;
+}
+
+struct hashmap *hashmap__new(hashmap_hash_fn hash_fn,
+			     hashmap_equal_fn equal_fn,
+			     void *ctx)
+{
+	struct hashmap *map = malloc(sizeof(struct hashmap));
+
+	if (!map)
+		return ERR_PTR(-ENOMEM);
+	hashmap__init(map, hash_fn, equal_fn, ctx);
+	return map;
+}
+
+void hashmap__clear(struct hashmap *map)
+{
+	struct hashmap_entry *cur, *tmp;
+	size_t bkt;
+
+	hashmap__for_each_entry_safe(map, cur, tmp, bkt) {
+		free(cur);
+	}
+	free(map->buckets);
+	map->buckets = NULL;
+	map->cap = map->cap_bits = map->sz = 0;
+}
+
+void hashmap__free(struct hashmap *map)
+{
+	if (!map)
+		return;
+
+	hashmap__clear(map);
+	free(map);
+}
+
+size_t hashmap__size(const struct hashmap *map)
+{
+	return map->sz;
+}
+
+size_t hashmap__capacity(const struct hashmap *map)
+{
+	return map->cap;
+}
+
+static bool hashmap_needs_to_grow(struct hashmap *map)
+{
+	/* grow if empty or more than 75% filled */
+	return (map->cap == 0) || ((map->sz + 1) * 4 / 3 > map->cap);
+}
+
+static int hashmap_grow(struct hashmap *map)
+{
+	struct hashmap_entry **new_buckets;
+	struct hashmap_entry *cur, *tmp;
+	size_t new_cap_bits, new_cap;
+	size_t h, bkt;
+
+	new_cap_bits = map->cap_bits + 1;
+	if (new_cap_bits < HASHMAP_MIN_CAP_BITS)
+		new_cap_bits = HASHMAP_MIN_CAP_BITS;
+
+	new_cap = 1UL << new_cap_bits;
+	new_buckets = calloc(new_cap, sizeof(new_buckets[0]));
+	if (!new_buckets)
+		return -ENOMEM;
+
+	hashmap__for_each_entry_safe(map, cur, tmp, bkt) {
+		h = hash_bits(map->hash_fn(cur->key, map->ctx), new_cap_bits);
+		hashmap_add_entry(&new_buckets[h], cur);
+	}
+
+	map->cap = new_cap;
+	map->cap_bits = new_cap_bits;
+	free(map->buckets);
+	map->buckets = new_buckets;
+
+	return 0;
+}
+
+static bool hashmap_find_entry(const struct hashmap *map,
+			       const void *key, size_t hash,
+			       struct hashmap_entry ***pprev,
+			       struct hashmap_entry **entry)
+{
+	struct hashmap_entry *cur, **prev_ptr;
+
+	if (!map->buckets)
+		return false;
+
+	for (prev_ptr = &map->buckets[hash], cur = *prev_ptr;
+	     cur;
+	     prev_ptr = &cur->next, cur = cur->next) {
+		if (map->equal_fn(cur->key, key, map->ctx)) {
+			if (pprev)
+				*pprev = prev_ptr;
+			*entry = cur;
+			return true;
+		}
+	}
+
+	return false;
+}
+
+int hashmap__insert(struct hashmap *map, const void *key, void *value,
+		    enum hashmap_insert_strategy strategy,
+		    const void **old_key, void **old_value)
+{
+	struct hashmap_entry *entry;
+	size_t h;
+	int err;
+
+	if (old_key)
+		*old_key = NULL;
+	if (old_value)
+		*old_value = NULL;
+
+	h = hash_bits(map->hash_fn(key, map->ctx), map->cap_bits);
+	if (strategy != HASHMAP_APPEND &&
+	    hashmap_find_entry(map, key, h, NULL, &entry)) {
+		if (old_key)
+			*old_key = entry->key;
+		if (old_value)
+			*old_value = entry->value;
+
+		if (strategy == HASHMAP_SET || strategy == HASHMAP_UPDATE) {
+			entry->key = key;
+			entry->value = value;
+			return 0;
+		} else if (strategy == HASHMAP_ADD) {
+			return -EEXIST;
+		}
+	}
+
+	if (strategy == HASHMAP_UPDATE)
+		return -ENOENT;
+
+	if (hashmap_needs_to_grow(map)) {
+		err = hashmap_grow(map);
+		if (err)
+			return err;
+		h = hash_bits(map->hash_fn(key, map->ctx), map->cap_bits);
+	}
+
+	entry = malloc(sizeof(struct hashmap_entry));
+	if (!entry)
+		return -ENOMEM;
+
+	entry->key = key;
+	entry->value = value;
+	hashmap_add_entry(&map->buckets[h], entry);
+	map->sz++;
+
+	return 0;
+}
+
+bool hashmap__find(const struct hashmap *map, const void *key, void **value)
+{
+	struct hashmap_entry *entry;
+	size_t h;
+
+	h = hash_bits(map->hash_fn(key, map->ctx), map->cap_bits);
+	if (!hashmap_find_entry(map, key, h, NULL, &entry))
+		return false;
+
+	if (value)
+		*value = entry->value;
+	return true;
+}
+
+bool hashmap__delete(struct hashmap *map, const void *key,
+		     const void **old_key, void **old_value)
+{
+	struct hashmap_entry **pprev, *entry;
+	size_t h;
+
+	h = hash_bits(map->hash_fn(key, map->ctx), map->cap_bits);
+	if (!hashmap_find_entry(map, key, h, &pprev, &entry))
+		return false;
+
+	if (old_key)
+		*old_key = entry->key;
+	if (old_value)
+		*old_value = entry->value;
+
+	hashmap_del_entry(pprev, entry);
+	free(entry);
+	map->sz--;
+
+	return true;
+}
+
diff --git a/tools/perf/util/hashmap.h b/tools/perf/util/hashmap.h
new file mode 100644
index 000000000000..e823b35e7371
--- /dev/null
+++ b/tools/perf/util/hashmap.h
@@ -0,0 +1,177 @@
+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
+
+/*
+ * Generic non-thread safe hash map implementation.
+ *
+ * Copyright (c) 2019 Facebook
+ */
+#ifndef __LIBBPF_HASHMAP_H
+#define __LIBBPF_HASHMAP_H
+
+#include <stdbool.h>
+#include <stddef.h>
+#ifdef __GLIBC__
+#include <bits/wordsize.h>
+#else
+#include <bits/reg.h>
+#endif
+
+static inline size_t hash_bits(size_t h, int bits)
+{
+	/* shuffle bits and return requested number of upper bits */
+	return (h * 11400714819323198485llu) >> (__WORDSIZE - bits);
+}
+
+typedef size_t (*hashmap_hash_fn)(const void *key, void *ctx);
+typedef bool (*hashmap_equal_fn)(const void *key1, const void *key2, void *ctx);
+
+struct hashmap_entry {
+	const void *key;
+	void *value;
+	struct hashmap_entry *next;
+};
+
+struct hashmap {
+	hashmap_hash_fn hash_fn;
+	hashmap_equal_fn equal_fn;
+	void *ctx;
+
+	struct hashmap_entry **buckets;
+	size_t cap;
+	size_t cap_bits;
+	size_t sz;
+};
+
+#define HASHMAP_INIT(hash_fn, equal_fn, ctx) {	\
+	.hash_fn = (hash_fn),			\
+	.equal_fn = (equal_fn),			\
+	.ctx = (ctx),				\
+	.buckets = NULL,			\
+	.cap = 0,				\
+	.cap_bits = 0,				\
+	.sz = 0,				\
+}
+
+void hashmap__init(struct hashmap *map, hashmap_hash_fn hash_fn,
+		   hashmap_equal_fn equal_fn, void *ctx);
+struct hashmap *hashmap__new(hashmap_hash_fn hash_fn,
+			     hashmap_equal_fn equal_fn,
+			     void *ctx);
+void hashmap__clear(struct hashmap *map);
+void hashmap__free(struct hashmap *map);
+
+size_t hashmap__size(const struct hashmap *map);
+size_t hashmap__capacity(const struct hashmap *map);
+
+/*
+ * Hashmap insertion strategy:
+ * - HASHMAP_ADD - only add key/value if key doesn't exist yet;
+ * - HASHMAP_SET - add key/value pair if key doesn't exist yet; otherwise,
+ *   update value;
+ * - HASHMAP_UPDATE - update value, if key already exists; otherwise, do
+ *   nothing and return -ENOENT;
+ * - HASHMAP_APPEND - always add key/value pair, even if key already exists.
+ *   This turns hashmap into a multimap by allowing multiple values to be
+ *   associated with the same key. Most useful read API for such hashmap is
+ *   hashmap__for_each_key_entry() iteration. If hashmap__find() is still
+ *   used, it will return last inserted key/value entry (first in a bucket
+ *   chain).
+ */
+enum hashmap_insert_strategy {
+	HASHMAP_ADD,
+	HASHMAP_SET,
+	HASHMAP_UPDATE,
+	HASHMAP_APPEND,
+};
+
+/*
+ * hashmap__insert() adds key/value entry w/ various semantics, depending on
+ * provided strategy value. If a given key/value pair replaced already
+ * existing key/value pair, both old key and old value will be returned
+ * through old_key and old_value to allow calling code do proper memory
+ * management.
+ */
+int hashmap__insert(struct hashmap *map, const void *key, void *value,
+		    enum hashmap_insert_strategy strategy,
+		    const void **old_key, void **old_value);
+
+static inline int hashmap__add(struct hashmap *map,
+			       const void *key, void *value)
+{
+	return hashmap__insert(map, key, value, HASHMAP_ADD, NULL, NULL);
+}
+
+static inline int hashmap__set(struct hashmap *map,
+			       const void *key, void *value,
+			       const void **old_key, void **old_value)
+{
+	return hashmap__insert(map, key, value, HASHMAP_SET,
+			       old_key, old_value);
+}
+
+static inline int hashmap__update(struct hashmap *map,
+				  const void *key, void *value,
+				  const void **old_key, void **old_value)
+{
+	return hashmap__insert(map, key, value, HASHMAP_UPDATE,
+			       old_key, old_value);
+}
+
+static inline int hashmap__append(struct hashmap *map,
+				  const void *key, void *value)
+{
+	return hashmap__insert(map, key, value, HASHMAP_APPEND, NULL, NULL);
+}
+
+bool hashmap__delete(struct hashmap *map, const void *key,
+		     const void **old_key, void **old_value);
+
+bool hashmap__find(const struct hashmap *map, const void *key, void **value);
+
+/*
+ * hashmap__for_each_entry - iterate over all entries in hashmap
+ * @map: hashmap to iterate
+ * @cur: struct hashmap_entry * used as a loop cursor
+ * @bkt: integer used as a bucket loop cursor
+ */
+#define hashmap__for_each_entry(map, cur, bkt)				    \
+	for (bkt = 0; bkt < map->cap; bkt++)				    \
+		for (cur = map->buckets[bkt]; cur; cur = cur->next)
+
+/*
+ * hashmap__for_each_entry_safe - iterate over all entries in hashmap, safe
+ * against removals
+ * @map: hashmap to iterate
+ * @cur: struct hashmap_entry * used as a loop cursor
+ * @tmp: struct hashmap_entry * used as a temporary next cursor storage
+ * @bkt: integer used as a bucket loop cursor
+ */
+#define hashmap__for_each_entry_safe(map, cur, tmp, bkt)		    \
+	for (bkt = 0; bkt < map->cap; bkt++)				    \
+		for (cur = map->buckets[bkt];				    \
+		     cur && ({tmp = cur->next; true; });		    \
+		     cur = tmp)
+
+/*
+ * hashmap__for_each_key_entry - iterate over entries associated with given key
+ * @map: hashmap to iterate
+ * @cur: struct hashmap_entry * used as a loop cursor
+ * @key: key to iterate entries for
+ */
+#define hashmap__for_each_key_entry(map, cur, _key)			    \
+	for (cur = ({ size_t bkt = hash_bits(map->hash_fn((_key), map->ctx),\
+					     map->cap_bits);		    \
+		     map->buckets ? map->buckets[bkt] : NULL; });	    \
+	     cur;							    \
+	     cur = cur->next)						    \
+		if (map->equal_fn(cur->key, (_key), map->ctx))
+
+#define hashmap__for_each_key_entry_safe(map, cur, tmp, _key)		    \
+	for (cur = ({ size_t bkt = hash_bits(map->hash_fn((_key), map->ctx),\
+					     map->cap_bits);		    \
+		     cur = map->buckets ? map->buckets[bkt] : NULL; });	    \
+	     cur && ({ tmp = cur->next; true; });			    \
+	     cur = tmp)							    \
+		if (map->equal_fn(cur->key, (_key), map->ctx))
+
+#endif /* __LIBBPF_HASHMAP_H */
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH v2 5/7] perf test: Provide a subtest callback to ask for the reason for skipping a subtest

[Ian Rogers]

Now subtests can inform why a test was skipped. The upcoming patch
improvint PMU event metric testing will use it.

Signed-off-by: Ian Rogers <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: John Garry <john.garry@huawei.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Clarke <pc@us.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lore.kernel.org/lkml/20200513212933.41273-1-irogers@google.com
[ split from a larger patch ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/builtin-test.c | 11 +++++++++--
 tools/perf/tests/tests.h        |  1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 3471ec52ea11..baee735e6aa5 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -429,8 +429,15 @@ static int test_and_print(struct test *t, bool force_skip, int subtest)
 	case TEST_OK:
 		pr_info(" Ok\n");
 		break;
-	case TEST_SKIP:
-		color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip\n");
+	case TEST_SKIP: {
+		const char *skip_reason = NULL;
+		if (t->subtest.skip_reason)
+			skip_reason = t->subtest.skip_reason(subtest);
+		if (skip_reason)
+			color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip (%s)\n", skip_reason);
+		else
+			color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip\n");
+	}
 		break;
 	case TEST_FAIL:
 	default:
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index d6d4ac34eeb7..88e45aeab94f 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -34,6 +34,7 @@ struct test {
 		bool skip_if_fail;
 		int (*get_nr)(void);
 		const char *(*get_desc)(int subtest);
+		const char *(*skip_reason)(int subtest);
 	} subtest;
 	bool (*is_supported)(void);
 	void *priv;
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH v2 6/7] perf test: Improve pmu event metric testing

[Ian Rogers]

Break pmu-events test into 2 and add a test to verify that all pmu
metric expressions simply parse. Try to parse all metric ids/events,
skip/warn if metrics for the current architecture fail to parse. To
support warning for a skip, and an ability for a subtest to describe why
it skips.

Tested on power9, skylakex, haswell, broadwell, westmere, sandybridge and
ivybridge.

May skip/warn on other architectures if metrics are invalid. In
particular s390 is untested, but its expressions are trivial. The
untested architectures with expressions are power8, cascadelakex,
tremontx, skylake, jaketown, ivytown and variants of haswell and
broadwell.

v3. addresses review comments from John Garry <john.garry@huawei.com>,
Jiri Olsa <jolsa@redhat.com> and Arnaldo Carvalho de Melo
<acme@kernel.org>.
v2. changes the commit message as event parsing errors no longer cause
the test to fail.

Signed-off-by: Ian Rogers <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: John Garry <john.garry@huawei.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Clarke <pc@us.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lore.kernel.org/lkml/20200513212933.41273-1-irogers@google.com
[ split from a larger patch ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/builtin-test.c |   7 ++
 tools/perf/tests/pmu-events.c   | 168 ++++++++++++++++++++++++++++++--
 tools/perf/tests/tests.h        |   3 +
 3 files changed, 172 insertions(+), 6 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index baee735e6aa5..9553f8061772 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -75,6 +75,13 @@ static struct test generic_tests[] = {
 	{
 		.desc = "PMU events",
 		.func = test__pmu_events,
+		.subtest = {
+			.skip_if_fail	= false,
+			.get_nr		= test__pmu_events_subtest_get_nr,
+			.get_desc	= test__pmu_events_subtest_get_desc,
+			.skip_reason	= test__pmu_events_subtest_skip_reason,
+		},
+
 	},
 	{
 		.desc = "DSO data read",
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index d64261da8bf7..e21f0addcfbb 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -8,6 +8,9 @@
 #include <linux/zalloc.h>
 #include "debug.h"
 #include "../pmu-events/pmu-events.h"
+#include "util/evlist.h"
+#include "util/expr.h"
+#include "util/parse-events.h"
 
 struct perf_pmu_test_event {
 	struct pmu_event event;
@@ -144,7 +147,7 @@ static struct pmu_events_map *__test_pmu_get_events_map(void)
 }
 
 /* Verify generated events from pmu-events.c is as expected */
-static int __test_pmu_event_table(void)
+static int test_pmu_event_table(void)
 {
 	struct pmu_events_map *map = __test_pmu_get_events_map();
 	struct pmu_event *table;
@@ -347,14 +350,11 @@ static int __test__pmu_event_aliases(char *pmu_name, int *count)
 	return res;
 }
 
-int test__pmu_events(struct test *test __maybe_unused,
-		     int subtest __maybe_unused)
+
+static int test_aliases(void)
 {
 	struct perf_pmu *pmu = NULL;
 
-	if (__test_pmu_event_table())
-		return -1;
-
 	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
 		int count = 0;
 
@@ -377,3 +377,159 @@ int test__pmu_events(struct test *test __maybe_unused,
 
 	return 0;
 }
+
+static bool is_number(const char *str)
+{
+	char *end_ptr;
+
+	strtod(str, &end_ptr);
+	return end_ptr != str;
+}
+
+static int check_parse_id(const char *id, bool same_cpu, struct pmu_event *pe)
+{
+	struct parse_events_error error;
+	struct evlist *evlist;
+	int ret;
+
+	/* Numbers are always valid. */
+	if (is_number(id))
+		return 0;
+
+	evlist = evlist__new();
+	memset(&error, 0, sizeof(error));
+	ret = parse_events(evlist, id, &error);
+	if (ret && same_cpu) {
+		pr_warning("Parse event failed metric '%s' id '%s' expr '%s'\n",
+			pe->metric_name, id, pe->metric_expr);
+		pr_warning("Error string '%s' help '%s'\n", error.str,
+			error.help);
+	} else if (ret) {
+		pr_debug3("Parse event failed, but for an event that may not be supported by this CPU.\nid '%s' metric '%s' expr '%s'\n",
+			  id, pe->metric_name, pe->metric_expr);
+		ret = 0;
+	}
+	evlist__delete(evlist);
+	free(error.str);
+	free(error.help);
+	free(error.first_str);
+	free(error.first_help);
+	return ret;
+}
+
+static void expr_failure(const char *msg,
+			 const struct pmu_events_map *map,
+			 const struct pmu_event *pe)
+{
+	pr_debug("%s for map %s %s %s\n",
+		msg, map->cpuid, map->version, map->type);
+	pr_debug("On metric %s\n", pe->metric_name);
+	pr_debug("On expression %s\n", pe->metric_expr);
+}
+
+static int test_parsing(void)
+{
+	struct pmu_events_map *cpus_map = perf_pmu__find_map(NULL);
+	struct pmu_events_map *map;
+	struct pmu_event *pe;
+	int i, j, k;
+	const char **ids;
+	int idnum;
+	int ret = 0;
+	struct expr_parse_ctx ctx;
+	double result;
+
+	i = 0;
+	for (;;) {
+		map = &pmu_events_map[i++];
+		if (!map->table)
+			break;
+		j = 0;
+		for (;;) {
+			pe = &map->table[j++];
+			if (!pe->name && !pe->metric_group && !pe->metric_name)
+				break;
+			if (!pe->metric_expr)
+				continue;
+			if (expr__find_other(pe->metric_expr, NULL,
+						&ids, &idnum, 0) < 0) {
+				expr_failure("Parse other failed", map, pe);
+				ret++;
+				continue;
+			}
+			expr__ctx_init(&ctx);
+
+			/*
+			 * Add all ids with a made up value. The value may
+			 * trigger divide by zero when subtracted and so try to
+			 * make them unique.
+			 */
+			for (k = 0; k < idnum; k++)
+				expr__add_id(&ctx, ids[k], k + 1);
+
+			for (k = 0; k < idnum; k++) {
+				if (check_parse_id(ids[k], map == cpus_map, pe))
+					ret++;
+			}
+
+			if (expr__parse(&result, &ctx, pe->metric_expr, 0)) {
+				expr_failure("Parse failed", map, pe);
+				ret++;
+			}
+			for (k = 0; k < idnum; k++)
+				zfree(&ids[k]);
+			free(ids);
+		}
+	}
+	/* TODO: fail when not ok */
+	return ret == 0 ? TEST_OK : TEST_SKIP;
+}
+
+static const struct {
+	int (*func)(void);
+	const char *desc;
+} pmu_events_testcase_table[] = {
+	{
+		.func = test_pmu_event_table,
+		.desc = "PMU event table sanity",
+	},
+	{
+		.func = test_aliases,
+		.desc = "PMU event map aliases",
+	},
+	{
+		.func = test_parsing,
+		.desc = "Parsing of PMU event table metrics",
+	},
+};
+
+const char *test__pmu_events_subtest_get_desc(int subtest)
+{
+	if (subtest < 0 ||
+	    subtest >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+		return NULL;
+	return pmu_events_testcase_table[subtest].desc;
+}
+
+const char *test__pmu_events_subtest_skip_reason(int subtest)
+{
+	if (subtest < 0 ||
+	    subtest >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+		return NULL;
+	if (pmu_events_testcase_table[subtest].func != test_parsing)
+		return NULL;
+	return "some metrics failed";
+}
+
+int test__pmu_events_subtest_get_nr(void)
+{
+	return (int)ARRAY_SIZE(pmu_events_testcase_table);
+}
+
+int test__pmu_events(struct test *test __maybe_unused, int subtest)
+{
+	if (subtest < 0 ||
+	    subtest >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+		return TEST_FAIL;
+	return pmu_events_testcase_table[subtest].func();
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 88e45aeab94f..6c6c4b6a4796 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -51,6 +51,9 @@ int test__perf_evsel__tp_sched_test(struct test *test, int subtest);
 int test__syscall_openat_tp_fields(struct test *test, int subtest);
 int test__pmu(struct test *test, int subtest);
 int test__pmu_events(struct test *test, int subtest);
+const char *test__pmu_events_subtest_get_desc(int subtest);
+const char *test__pmu_events_subtest_skip_reason(int subtest);
+int test__pmu_events_subtest_get_nr(void);
 int test__attr(struct test *test, int subtest);
 int test__dso_data(struct test *test, int subtest);
 int test__dso_data_cache(struct test *test, int subtest);
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH v2 7/7] perf expr: Migrate expr ids table to a hashmap

[Ian Rogers]

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

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

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/expr.c       |  40 ++++++-----
 tools/perf/tests/pmu-events.c |  25 +++----
 tools/perf/util/expr.c        | 129 +++++++++++++++++++---------------
 tools/perf/util/expr.h        |  26 +++----
 tools/perf/util/expr.y        |  22 +-----
 tools/perf/util/metricgroup.c |  87 +++++++++++------------
 tools/perf/util/stat-shadow.c |  49 ++++++++-----
 7 files changed, 197 insertions(+), 181 deletions(-)

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

Re: [PATCH v2 0/7] Copy hashmap to tools/perf/util, use in perf expr

[Arnaldo Carvalho de Melo]

Em Fri, May 15, 2020 at 09:50:00AM -0700, Ian Rogers escreveu:
> Perf's expr code currently builds an array of strings then removes
> duplicates. The array is larger than necessary and has recently been
> increased in size. When this was done it was commented that a hashmap
> would be preferable.
> 
> libbpf has a hashmap but libbpf isn't currently required to build
> perf. To satisfy various concerns this change copies libbpf's hashmap
> into tools/perf/util, it then adds a check in perf that the two are in
> sync.
> 
> Andrii's patch to hashmap from bpf-next is brought into this set to
> fix issues with hashmap__clear.
> 
> Two minor changes to libbpf's hashmap are made that remove an unused
> dependency and fix a compiler warning.

Andrii/Alexei/Daniel, what do you think about me merging these fixes in my
perf-tools-next branch?

- Arnaldo
 
> Two perf test changes are also brought in as they need refactoring to
> account for the expr API change and it is expected they will land
> ahead of this.
> https://lore.kernel.org/lkml/20200513062236.854-2-irogers@google.com/
> 
> Tested with 'perf test' and 'make -C tools/perf build-test'.
> 
> The hashmap change was originally part of an RFC:
> https://lore.kernel.org/lkml/20200508053629.210324-1-irogers@google.com/
> 
> v2. moves hashmap into tools/perf/util rather than libapi, to allow
> hashmap's libbpf symbols to be visible when built statically for
> testing.
> 
> Andrii Nakryiko (1):
>   libbpf: Fix memory leak and possible double-free in hashmap__clear
> 
> Ian Rogers (6):
>   libbpf hashmap: Remove unused #include
>   libbpf hashmap: Fix signedness warnings
>   tools lib/api: Copy libbpf hashmap to tools/perf/util
>   perf test: Provide a subtest callback to ask for the reason for
>     skipping a subtest
>   perf test: Improve pmu event metric testing
>   perf expr: Migrate expr ids table to a hashmap
> 
>  tools/lib/bpf/hashmap.c         |  10 +-
>  tools/lib/bpf/hashmap.h         |   1 -
>  tools/perf/check-headers.sh     |   4 +
>  tools/perf/tests/builtin-test.c |  18 ++-
>  tools/perf/tests/expr.c         |  40 +++---
>  tools/perf/tests/pmu-events.c   | 169 ++++++++++++++++++++++-
>  tools/perf/tests/tests.h        |   4 +
>  tools/perf/util/Build           |   4 +
>  tools/perf/util/expr.c          | 129 +++++++++--------
>  tools/perf/util/expr.h          |  26 ++--
>  tools/perf/util/expr.y          |  22 +--
>  tools/perf/util/hashmap.c       | 238 ++++++++++++++++++++++++++++++++
>  tools/perf/util/hashmap.h       | 177 ++++++++++++++++++++++++
>  tools/perf/util/metricgroup.c   |  87 ++++++------
>  tools/perf/util/stat-shadow.c   |  49 ++++---
>  15 files changed, 798 insertions(+), 180 deletions(-)
>  create mode 100644 tools/perf/util/hashmap.c
>  create mode 100644 tools/perf/util/hashmap.h
> 
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

-- 

- Arnaldo

Re: [PATCH v2 7/7] perf expr: Migrate expr ids table to a hashmap

[Andrii Nakryiko]

On Fri, May 15, 2020 at 9:51 AM Ian Rogers <irogers@google.com> wrote:
>
> Use a hashmap between a char* string and a double* value. While bpf's
> hashmap entries are size_t in size, we can't guarantee sizeof(size_t) >=
> sizeof(double). Avoid a memory allocation when gathering ids by making 0.0
> a special value encoded as NULL.
>
> Original map suggestion by Andi Kleen:
> https://lore.kernel.org/lkml/20200224210308.GQ160988@tassilo.jf.intel.com/
> and seconded by Jiri Olsa:
> https://lore.kernel.org/lkml/20200423112915.GH1136647@krava/
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/expr.c       |  40 ++++++-----
>  tools/perf/tests/pmu-events.c |  25 +++----
>  tools/perf/util/expr.c        | 129 +++++++++++++++++++---------------
>  tools/perf/util/expr.h        |  26 +++----
>  tools/perf/util/expr.y        |  22 +-----
>  tools/perf/util/metricgroup.c |  87 +++++++++++------------
>  tools/perf/util/stat-shadow.c |  49 ++++++++-----
>  7 files changed, 197 insertions(+), 181 deletions(-)
>
> diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> index 3f742612776a..5e606fd5a2c6 100644
> --- a/tools/perf/tests/expr.c
> +++ b/tools/perf/tests/expr.c
> @@ -19,11 +19,9 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
>  int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
>  {
>         const char *p;
> -       const char **other;
> -       double val;
> -       int i, ret;
> +       double val, *val_ptr;
> +       int ret;
>         struct expr_parse_ctx ctx;
> -       int num_other;
>
>         expr__ctx_init(&ctx);
>         expr__add_id(&ctx, "FOO", 1);
> @@ -52,25 +50,29 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
>         ret = expr__parse(&val, &ctx, p, 1);
>         TEST_ASSERT_VAL("missing operand", ret == -1);
>
> +       hashmap__clear(&ctx.ids);

hashmap__clear() will free up memory allocated for hashmap itself and
hash entries, but not keys/values. Unless it's happening somewhere
else, you'll need to do something similar to expr__ctx_clear() below?

Same below for another "lone" hashmap_clear() call.

>         TEST_ASSERT_VAL("find other",
> -                       expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", &other, &num_other, 1) == 0);

[...]

Re: [PATCH v2 2/7] libbpf hashmap: Remove unused #include

[Andrii Nakryiko]

On Fri, May 15, 2020 at 9:51 AM Ian Rogers <irogers@google.com> wrote:
>
> Remove #include of libbpf_internal.h that is unused.
> Discussed in this thread:
> https://lore.kernel.org/lkml/CAEf4BzZRmiEds_8R8g4vaAeWvJzPb4xYLnpF0X2VNY8oTzkphQ@mail.gmail.com/
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/lib/bpf/hashmap.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h
> index bae8879cdf58..e823b35e7371 100644
> --- a/tools/lib/bpf/hashmap.h
> +++ b/tools/lib/bpf/hashmap.h
> @@ -15,7 +15,6 @@
>  #else
>  #include <bits/reg.h>
>  #endif
> -#include "libbpf_internal.h"
>
>  static inline size_t hash_bits(size_t h, int bits)
>  {
> --
> 2.26.2.761.g0e0b3e54be-goog
>

Re: [PATCH v2 3/7] libbpf hashmap: Fix signedness warnings

[Andrii Nakryiko]

On Fri, May 15, 2020 at 9:51 AM Ian Rogers <irogers@google.com> wrote:
>
> Fixes the following warnings:
>
> hashmap.c: In function ‘hashmap__clear’:
> hashmap.h:150:20: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
>   150 |  for (bkt = 0; bkt < map->cap; bkt++)        \
>
> hashmap.c: In function ‘hashmap_grow’:
> hashmap.h:150:20: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
>   150 |  for (bkt = 0; bkt < map->cap; bkt++)        \
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/lib/bpf/hashmap.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/hashmap.c b/tools/lib/bpf/hashmap.c
> index cffb96202e0d..a405dad068f5 100644
> --- a/tools/lib/bpf/hashmap.c
> +++ b/tools/lib/bpf/hashmap.c
> @@ -60,7 +60,7 @@ struct hashmap *hashmap__new(hashmap_hash_fn hash_fn,
>  void hashmap__clear(struct hashmap *map)
>  {
>         struct hashmap_entry *cur, *tmp;
> -       int bkt;
> +       size_t bkt;
>
>         hashmap__for_each_entry_safe(map, cur, tmp, bkt) {
>                 free(cur);
> @@ -100,8 +100,7 @@ static int hashmap_grow(struct hashmap *map)
>         struct hashmap_entry **new_buckets;
>         struct hashmap_entry *cur, *tmp;
>         size_t new_cap_bits, new_cap;
> -       size_t h;
> -       int bkt;
> +       size_t h, bkt;
>
>         new_cap_bits = map->cap_bits + 1;
>         if (new_cap_bits < HASHMAP_MIN_CAP_BITS)
> --
> 2.26.2.761.g0e0b3e54be-goog
>

Re: [PATCH v2 7/7] perf expr: Migrate expr ids table to a hashmap

[Jiri Olsa]

On Fri, May 15, 2020 at 09:50:07AM -0700, Ian Rogers wrote:

SNIP

> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index b071df373f8b..37be5a368d6e 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -85,8 +85,7 @@ static void metricgroup__rblist_init(struct rblist *metric_events)
>  
>  struct egroup {
>  	struct list_head nd;
> -	int idnum;
> -	const char **ids;
> +	struct expr_parse_ctx pctx;
>  	const char *metric_name;
>  	const char *metric_expr;
>  	const char *metric_unit;
> @@ -94,19 +93,21 @@ struct egroup {
>  };
>  
>  static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> -				      const char **ids,
> -				      int idnum,
> +				      struct expr_parse_ctx *pctx,
>  				      struct evsel **metric_events,
>  				      bool *evlist_used)
>  {
>  	struct evsel *ev;
> -	int i = 0, j = 0;
>  	bool leader_found;
> +	const size_t idnum = hashmap__size(&pctx->ids);
> +	size_t i = 0;
> +	int j = 0;
> +	double *val_ptr;
>  
>  	evlist__for_each_entry (perf_evlist, ev) {
>  		if (evlist_used[j++])
>  			continue;
> -		if (!strcmp(ev->name, ids[i])) {
> +		if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {

hum, you sure it's doing the same thing as before?

hashmap__find will succede all the time in here, while the
previous code was looking for the start of the group ...
the logic in here is little convoluted, so maybe I'm
missing some point in here ;-)

jirka

>  			if (!metric_events[i])
>  				metric_events[i] = ev;
>  			i++;
> @@ -118,7 +119,8 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>  			memset(metric_events, 0,
>  				sizeof(struct evsel *) * idnum);
>  
> -			if (!strcmp(ev->name, ids[i])) {
> +			if (hashmap__find(&pctx->ids, ev->name,
> +					  (void **)&val_ptr)) {
>  				if (!metric_events[i])
>  					metric_events[i] = ev;

SNIP

Re: [PATCH v2 4/7] tools lib/api: Copy libbpf hashmap to tools/perf/util

[Andrii Nakryiko]

On Fri, May 15, 2020 at 9:50 AM Ian Rogers <irogers@google.com> wrote:
>
> Allow use of hashmap in perf. Modify perf's check-headers.sh script to
> check that the files are kept in sync, in the same way kernel headers are
> checked. This will warn if they are out of sync at the start of a perf
> build.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---

Given you want to make sure they stay 1 to 1, would just creating a
symlink work instead of copying the code?

Either way, I think hashmap is stable and not going to change
frequently, so whichever way is fine with me.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/perf/check-headers.sh |   4 +
>  tools/perf/util/Build       |   4 +
>  tools/perf/util/hashmap.c   | 238 ++++++++++++++++++++++++++++++++++++
>  tools/perf/util/hashmap.h   | 177 +++++++++++++++++++++++++++
>  4 files changed, 423 insertions(+)
>  create mode 100644 tools/perf/util/hashmap.c
>  create mode 100644 tools/perf/util/hashmap.h
>

[...]

Re: [PATCH v2 0/7] Copy hashmap to tools/perf/util, use in perf expr

[Andrii Nakryiko]

On Fri, May 15, 2020 at 10:01 AM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Fri, May 15, 2020 at 09:50:00AM -0700, Ian Rogers escreveu:
> > Perf's expr code currently builds an array of strings then removes
> > duplicates. The array is larger than necessary and has recently been
> > increased in size. When this was done it was commented that a hashmap
> > would be preferable.
> >
> > libbpf has a hashmap but libbpf isn't currently required to build
> > perf. To satisfy various concerns this change copies libbpf's hashmap
> > into tools/perf/util, it then adds a check in perf that the two are in
> > sync.
> >
> > Andrii's patch to hashmap from bpf-next is brought into this set to
> > fix issues with hashmap__clear.
> >
> > Two minor changes to libbpf's hashmap are made that remove an unused
> > dependency and fix a compiler warning.
>
> Andrii/Alexei/Daniel, what do you think about me merging these fixes in my
> perf-tools-next branch?

I'm ok with the idea, but it's up to maintainers to coordinate this :)

>
> - Arnaldo
>
> > Two perf test changes are also brought in as they need refactoring to
> > account for the expr API change and it is expected they will land
> > ahead of this.
> > https://lore.kernel.org/lkml/20200513062236.854-2-irogers@google.com/
> >
> > Tested with 'perf test' and 'make -C tools/perf build-test'.
> >
> > The hashmap change was originally part of an RFC:
> > https://lore.kernel.org/lkml/20200508053629.210324-1-irogers@google.com/
> >
> > v2. moves hashmap into tools/perf/util rather than libapi, to allow
> > hashmap's libbpf symbols to be visible when built statically for
> > testing.
> >
> > Andrii Nakryiko (1):
> >   libbpf: Fix memory leak and possible double-free in hashmap__clear
> >
> > Ian Rogers (6):
> >   libbpf hashmap: Remove unused #include
> >   libbpf hashmap: Fix signedness warnings
> >   tools lib/api: Copy libbpf hashmap to tools/perf/util
> >   perf test: Provide a subtest callback to ask for the reason for
> >     skipping a subtest
> >   perf test: Improve pmu event metric testing
> >   perf expr: Migrate expr ids table to a hashmap
> >
> >  tools/lib/bpf/hashmap.c         |  10 +-
> >  tools/lib/bpf/hashmap.h         |   1 -
> >  tools/perf/check-headers.sh     |   4 +
> >  tools/perf/tests/builtin-test.c |  18 ++-
> >  tools/perf/tests/expr.c         |  40 +++---
> >  tools/perf/tests/pmu-events.c   | 169 ++++++++++++++++++++++-
> >  tools/perf/tests/tests.h        |   4 +
> >  tools/perf/util/Build           |   4 +
> >  tools/perf/util/expr.c          | 129 +++++++++--------
> >  tools/perf/util/expr.h          |  26 ++--
> >  tools/perf/util/expr.y          |  22 +--
> >  tools/perf/util/hashmap.c       | 238 ++++++++++++++++++++++++++++++++
> >  tools/perf/util/hashmap.h       | 177 ++++++++++++++++++++++++
> >  tools/perf/util/metricgroup.c   |  87 ++++++------
> >  tools/perf/util/stat-shadow.c   |  49 ++++---
> >  15 files changed, 798 insertions(+), 180 deletions(-)
> >  create mode 100644 tools/perf/util/hashmap.c
> >  create mode 100644 tools/perf/util/hashmap.h
> >
> > --
> > 2.26.2.761.g0e0b3e54be-goog
> >
>
> --
>
> - Arnaldo

Re: [PATCH v2 0/7] Copy hashmap to tools/perf/util, use in perf expr

[arnaldo.melo]

<bpf@vger.kernel.org>,Stephane Eranian <eranian@google.com>
From: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Message-ID: <79BCBAF7-BF5F-4556-A923-56E9D82FB570@gmail.com>



On May 15, 2020 4:42:46 PM GMT-03:00, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>On Fri, May 15, 2020 at 10:01 AM Arnaldo Carvalho de Melo
><arnaldo.melo@gmail.com> wrote:
>>
>> Em Fri, May 15, 2020 at 09:50:00AM -0700, Ian Rogers escreveu:
>> > Perf's expr code currently builds an array of strings then removes
>> > duplicates. The array is larger than necessary and has recently
>been
>> > increased in size. When this was done it was commented that a
>hashmap
>> > would be preferable.
>> >
>> > libbpf has a hashmap but libbpf isn't currently required to build
>> > perf. To satisfy various concerns this change copies libbpf's
>hashmap
>> > into tools/perf/util, it then adds a check in perf that the two are
>in
>> > sync.
>> >
>> > Andrii's patch to hashmap from bpf-next is brought into this set to
>> > fix issues with hashmap__clear.
>> >
>> > Two minor changes to libbpf's hashmap are made that remove an
>unused
>> > dependency and fix a compiler warning.
>>
>> Andrii/Alexei/Daniel, what do you think about me merging these fixes
>in my
>> perf-tools-next branch?
>
>I'm ok with the idea, but it's up to maintainers to coordinate this :)

Good to know, do I'll take all patches except the ones touching libppf, will just make sure the copy is done with the patches applied.

At some point they'll land in libbpf and the warning from check_headers.sh will be resolved.

Thanks,

- Arnaldo

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v2 7/7] perf expr: Migrate expr ids table to a hashmap

[Ian Rogers]

On Fri, May 15, 2020 at 12:41 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, May 15, 2020 at 09:50:07AM -0700, Ian Rogers wrote:
>
> SNIP
>
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index b071df373f8b..37be5a368d6e 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -85,8 +85,7 @@ static void metricgroup__rblist_init(struct rblist *metric_events)
> >
> >  struct egroup {
> >       struct list_head nd;
> > -     int idnum;
> > -     const char **ids;
> > +     struct expr_parse_ctx pctx;
> >       const char *metric_name;
> >       const char *metric_expr;
> >       const char *metric_unit;
> > @@ -94,19 +93,21 @@ struct egroup {
> >  };
> >
> >  static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > -                                   const char **ids,
> > -                                   int idnum,
> > +                                   struct expr_parse_ctx *pctx,
> >                                     struct evsel **metric_events,
> >                                     bool *evlist_used)
> >  {
> >       struct evsel *ev;
> > -     int i = 0, j = 0;
> >       bool leader_found;
> > +     const size_t idnum = hashmap__size(&pctx->ids);
> > +     size_t i = 0;
> > +     int j = 0;
> > +     double *val_ptr;
> >
> >       evlist__for_each_entry (perf_evlist, ev) {
> >               if (evlist_used[j++])
> >                       continue;
> > -             if (!strcmp(ev->name, ids[i])) {
> > +             if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
>
> hum, you sure it's doing the same thing as before?
>
> hashmap__find will succede all the time in here, while the
> previous code was looking for the start of the group ...
> the logic in here is little convoluted, so maybe I'm
> missing some point in here ;-)

If we have a metric like "A + B" and another like "C / D" then by
we'll generate a string (the extra_events strbuf in the code) like
"{A,B}:W,{C,D}:W" from __metricgroup__add_metric. This will turn into
an evlist in metricgroup__parse_groups of A,B,C,D. The code is trying
to associate the events A,B with the first metric and C,D with the
second. The code doesn't support sharing of events and events are
marked as used and can't be part of other metrics. The evlist order is
also reflective of the order of metrics, so if there were metrics "A +
B + C" and "A + B", as the first metric is first in the evlist we
don't run the risk of C being placed with A and B in a different
group.

The old code used the order of events to match within a metric and say
for metric "A+B+C" we want to match A then B, and so on. The new code
acts more like a set, so "A + B + C" becomes a set containing A, B and
C, we check A is in the set then B and then C. For both pieces of code
they are only working because of the evlist_used "bitmap" and that the
order in the evlists and metrics matches.

The current code could just use ordering to match first n1 events with
the first metric, the next n2 events with the second and so on. So
both the find now, and the strcmp before always return true in this
branch.

In the RFC patch set I want to share events and so I do checks related
to the group leader so that I know when moving from one group to
another in the evlist. The find/strcmp becomes load bearing as I will
re-use events as long as they match.
https://lore.kernel.org/lkml/20200508053629.210324-14-irogers@google.com/

> jirka
>
> >                       if (!metric_events[i])
> >                               metric_events[i] = ev;
> >                       i++;
> > @@ -118,7 +119,8 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >                       memset(metric_events, 0,
> >                               sizeof(struct evsel *) * idnum);

This re-check was unnecessary in the old code and unnecessary even
more so now as the hashmap_find is given exactly the same arguments.
I'll remove it in v3 while addressing Andrii's memory leak fixes.

Thanks,
Ian

> > -                     if (!strcmp(ev->name, ids[i])) {
> > +                     if (hashmap__find(&pctx->ids, ev->name,
> > +                                       (void **)&val_ptr)) {
> >                               if (!metric_events[i])
> >                                       metric_events[i] = ev;
>
> SNIP
>

Re: [PATCH v2 0/7] Copy hashmap to tools/perf/util, use in perf expr

[Ian Rogers]

On Fri, May 15, 2020 at 2:19 PM <arnaldo.melo@gmail.com> wrote:
>
> <bpf@vger.kernel.org>,Stephane Eranian <eranian@google.com>
> From: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> Message-ID: <79BCBAF7-BF5F-4556-A923-56E9D82FB570@gmail.com>
>
>
>
> On May 15, 2020 4:42:46 PM GMT-03:00, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >On Fri, May 15, 2020 at 10:01 AM Arnaldo Carvalho de Melo
> ><arnaldo.melo@gmail.com> wrote:
> >>
> >> Em Fri, May 15, 2020 at 09:50:00AM -0700, Ian Rogers escreveu:
> >> > Perf's expr code currently builds an array of strings then removes
> >> > duplicates. The array is larger than necessary and has recently
> >been
> >> > increased in size. When this was done it was commented that a
> >hashmap
> >> > would be preferable.
> >> >
> >> > libbpf has a hashmap but libbpf isn't currently required to build
> >> > perf. To satisfy various concerns this change copies libbpf's
> >hashmap
> >> > into tools/perf/util, it then adds a check in perf that the two are
> >in
> >> > sync.
> >> >
> >> > Andrii's patch to hashmap from bpf-next is brought into this set to
> >> > fix issues with hashmap__clear.
> >> >
> >> > Two minor changes to libbpf's hashmap are made that remove an
> >unused
> >> > dependency and fix a compiler warning.
> >>
> >> Andrii/Alexei/Daniel, what do you think about me merging these fixes
> >in my
> >> perf-tools-next branch?
> >
> >I'm ok with the idea, but it's up to maintainers to coordinate this :)
>
> Good to know, do I'll take all patches except the ones touching libppf, will just make sure the copy is done with the patches applied.
>
> At some point they'll land in libbpf and the warning from check_headers.sh will be resolved.

So tools/perf/util's hashmap will be ahead of libbpf's, as without the
fixes the perf build is broken by Werror. This will cause
check_headers to warn in perf builds, which would usually mean our
header was older than the source one, but in this case it means the
opposite, we're waiting for the libbpf patches to merge. Aside from
some interim warnings everything will resolve itself and Arnaldo
avoids landing patches in libbpf that can interfere with bpf-next.

It takes some getting your head around but sounds good to me :-) I
think the only workable alternatives would be to explore having a
single version of the code in some kind of shared libhashmap or to
implement another hashmap in libapi. I'd like to get the rest of this
work unblocked and so it'd be nice to land this and we can always
refactor later - I like Arnaldo's plan. Can a bpf maintainer make sure
the hashmap changes get pulled into bpf-next?

Thanks!
Ian

> Thanks,
>
> - Arnaldo
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v2 7/7] perf expr: Migrate expr ids table to a hashmap

[Ian Rogers]

On Fri, May 15, 2020 at 12:39 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, May 15, 2020 at 9:51 AM Ian Rogers <irogers@google.com> wrote:
> >
> > Use a hashmap between a char* string and a double* value. While bpf's
> > hashmap entries are size_t in size, we can't guarantee sizeof(size_t) >=
> > sizeof(double). Avoid a memory allocation when gathering ids by making 0.0
> > a special value encoded as NULL.
> >
> > Original map suggestion by Andi Kleen:
> > https://lore.kernel.org/lkml/20200224210308.GQ160988@tassilo.jf.intel.com/
> > and seconded by Jiri Olsa:
> > https://lore.kernel.org/lkml/20200423112915.GH1136647@krava/
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/tests/expr.c       |  40 ++++++-----
> >  tools/perf/tests/pmu-events.c |  25 +++----
> >  tools/perf/util/expr.c        | 129 +++++++++++++++++++---------------
> >  tools/perf/util/expr.h        |  26 +++----
> >  tools/perf/util/expr.y        |  22 +-----
> >  tools/perf/util/metricgroup.c |  87 +++++++++++------------
> >  tools/perf/util/stat-shadow.c |  49 ++++++++-----
> >  7 files changed, 197 insertions(+), 181 deletions(-)
> >
> > diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> > index 3f742612776a..5e606fd5a2c6 100644
> > --- a/tools/perf/tests/expr.c
> > +++ b/tools/perf/tests/expr.c
> > @@ -19,11 +19,9 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
> >  int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
> >  {
> >         const char *p;
> > -       const char **other;
> > -       double val;
> > -       int i, ret;
> > +       double val, *val_ptr;
> > +       int ret;
> >         struct expr_parse_ctx ctx;
> > -       int num_other;
> >
> >         expr__ctx_init(&ctx);
> >         expr__add_id(&ctx, "FOO", 1);
> > @@ -52,25 +50,29 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
> >         ret = expr__parse(&val, &ctx, p, 1);
> >         TEST_ASSERT_VAL("missing operand", ret == -1);
> >
> > +       hashmap__clear(&ctx.ids);
>
> hashmap__clear() will free up memory allocated for hashmap itself and
> hash entries, but not keys/values. Unless it's happening somewhere
> else, you'll need to do something similar to expr__ctx_clear() below?

In this case the const char* keys come from the literals added on
lines 27 and 28 and so didn't need free-ing - which is what
expr__ctx_clear() does. I've made these literals strdups and switched
to expr__ctx_clear() as you suggest, as this is more reflective of the
real use.

> Same below for another "lone" hashmap_clear() call.

This was a memory leak, thanks!
Ian

> >         TEST_ASSERT_VAL("find other",
> > -                       expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", &other, &num_other, 1) == 0);
>
> [...]

Re: [PATCH v2 7/7] perf expr: Migrate expr ids table to a hashmap

[Jiri Olsa]

On Fri, May 15, 2020 at 09:50:07AM -0700, Ian Rogers wrote:

SNIP

> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 8b4ce704a68d..f64ab91c432b 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -4,25 +4,76 @@
>  #include "expr.h"
>  #include "expr-bison.h"
>  #include "expr-flex.h"
> +#include <linux/kernel.h>
>  
>  #ifdef PARSER_DEBUG
>  extern int expr_debug;
>  #endif
>  
> +static size_t key_hash(const void *key, void *ctx __maybe_unused)
> +{
> +	const char *str = (const char *)key;
> +	size_t hash = 0;
> +
> +	while (*str != '\0') {
> +		hash *= 31;
> +		hash += *str;
> +		str++;
> +	}
> +	return hash;
> +}
> +
> +static bool key_equal(const void *key1, const void *key2,
> +		    void *ctx __maybe_unused)
> +{
> +	return !strcmp((const char *)key1, (const char *)key2);

should that be strcasecmp ? would it affect the key_hash as well?

jirka

Re: [PATCH v2 7/7] perf expr: Migrate expr ids table to a hashmap

[Jiri Olsa]

On Fri, May 15, 2020 at 02:35:43PM -0700, Ian Rogers wrote:
> On Fri, May 15, 2020 at 12:41 PM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Fri, May 15, 2020 at 09:50:07AM -0700, Ian Rogers wrote:
> >
> > SNIP
> >
> > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > > index b071df373f8b..37be5a368d6e 100644
> > > --- a/tools/perf/util/metricgroup.c
> > > +++ b/tools/perf/util/metricgroup.c
> > > @@ -85,8 +85,7 @@ static void metricgroup__rblist_init(struct rblist *metric_events)
> > >
> > >  struct egroup {
> > >       struct list_head nd;
> > > -     int idnum;
> > > -     const char **ids;
> > > +     struct expr_parse_ctx pctx;
> > >       const char *metric_name;
> > >       const char *metric_expr;
> > >       const char *metric_unit;
> > > @@ -94,19 +93,21 @@ struct egroup {
> > >  };
> > >
> > >  static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > > -                                   const char **ids,
> > > -                                   int idnum,
> > > +                                   struct expr_parse_ctx *pctx,
> > >                                     struct evsel **metric_events,
> > >                                     bool *evlist_used)
> > >  {
> > >       struct evsel *ev;
> > > -     int i = 0, j = 0;
> > >       bool leader_found;
> > > +     const size_t idnum = hashmap__size(&pctx->ids);
> > > +     size_t i = 0;
> > > +     int j = 0;
> > > +     double *val_ptr;
> > >
> > >       evlist__for_each_entry (perf_evlist, ev) {
> > >               if (evlist_used[j++])
> > >                       continue;
> > > -             if (!strcmp(ev->name, ids[i])) {
> > > +             if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
> >
> > hum, you sure it's doing the same thing as before?
> >
> > hashmap__find will succede all the time in here, while the
> > previous code was looking for the start of the group ...
> > the logic in here is little convoluted, so maybe I'm
> > missing some point in here ;-)
> 
> If we have a metric like "A + B" and another like "C / D" then by
> we'll generate a string (the extra_events strbuf in the code) like
> "{A,B}:W,{C,D}:W" from __metricgroup__add_metric. This will turn into
> an evlist in metricgroup__parse_groups of A,B,C,D. The code is trying
> to associate the events A,B with the first metric and C,D with the
> second. The code doesn't support sharing of events and events are
> marked as used and can't be part of other metrics. The evlist order is
> also reflective of the order of metrics, so if there were metrics "A +
> B + C" and "A + B", as the first metric is first in the evlist we
> don't run the risk of C being placed with A and B in a different
> group.
> 
> The old code used the order of events to match within a metric and say
> for metric "A+B+C" we want to match A then B, and so on. The new code
> acts more like a set, so "A + B + C" becomes a set containing A, B and
> C, we check A is in the set then B and then C. For both pieces of code
> they are only working because of the evlist_used "bitmap" and that the
> order in the evlists and metrics matches.
> 
> The current code could just use ordering to match first n1 events with
> the first metric, the next n2 events with the second and so on. So
> both the find now, and the strcmp before always return true in this
> branch.
> 
> In the RFC patch set I want to share events and so I do checks related
> to the group leader so that I know when moving from one group to
> another in the evlist. The find/strcmp becomes load bearing as I will
> re-use events as long as they match.
> https://lore.kernel.org/lkml/20200508053629.210324-14-irogers@google.com/
> 
> > jirka
> >
> > >                       if (!metric_events[i])
> > >                               metric_events[i] = ev;
> > >                       i++;
> > > @@ -118,7 +119,8 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > >                       memset(metric_events, 0,
> > >                               sizeof(struct evsel *) * idnum);
> 
> This re-check was unnecessary in the old code and unnecessary even
> more so now as the hashmap_find is given exactly the same arguments.
> I'll remove it in v3 while addressing Andrii's memory leak fixes.

ok, that was my other confusion, because it's useless ;-)

thanks for the explanation,
jirka

> 
> Thanks,
> Ian
> 
> > > -                     if (!strcmp(ev->name, ids[i])) {
> > > +                     if (hashmap__find(&pctx->ids, ev->name,
> > > +                                       (void **)&val_ptr)) {
> > >                               if (!metric_events[i])
> > >                                       metric_events[i] = ev;
> >
> > SNIP
> >
> 

Re: [PATCH v2 7/7] perf expr: Migrate expr ids table to a hashmap

[Ian Rogers]

On Fri, May 15, 2020 at 3:41 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, May 15, 2020 at 09:50:07AM -0700, Ian Rogers wrote:
>
> SNIP
>
> > diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> > index 8b4ce704a68d..f64ab91c432b 100644
> > --- a/tools/perf/util/expr.c
> > +++ b/tools/perf/util/expr.c
> > @@ -4,25 +4,76 @@
> >  #include "expr.h"
> >  #include "expr-bison.h"
> >  #include "expr-flex.h"
> > +#include <linux/kernel.h>
> >
> >  #ifdef PARSER_DEBUG
> >  extern int expr_debug;
> >  #endif
> >
> > +static size_t key_hash(const void *key, void *ctx __maybe_unused)
> > +{
> > +     const char *str = (const char *)key;
> > +     size_t hash = 0;
> > +
> > +     while (*str != '\0') {
> > +             hash *= 31;
> > +             hash += *str;
> > +             str++;
> > +     }
> > +     return hash;
> > +}
> > +
> > +static bool key_equal(const void *key1, const void *key2,
> > +                 void *ctx __maybe_unused)
> > +{
> > +     return !strcmp((const char *)key1, (const char *)key2);
>
> should that be strcasecmp ? would it affect the key_hash as well?

The original code does make use of strcasecmp in one place, but in the
group matching (the main useless use for this code) it doesn't. I
don't think it is a regression to keep it as this, and would like a
test case for when it does matter. Is that ok?

Thanks,
Ian

> jirka
>

Re: [PATCH v2 0/7] Copy hashmap to tools/perf/util, use in perf expr

[Daniel Borkmann]

On 5/15/20 11:18 PM, arnaldo.melo@gmail.com wrote:
[...]
>>> Andrii/Alexei/Daniel, what do you think about me merging these fixes
>> in my
>>> perf-tools-next branch?
>>
>> I'm ok with the idea, but it's up to maintainers to coordinate this :)
> 
> Good to know, do I'll take all patches except the ones touching libppf, will just make sure the copy is done with the patches applied.
> 
> At some point they'll land in libbpf and the warning from check_headers.sh will be resolved.

Works for me, I've just taken in Ian's two libbpf ones into bpf-next.

Thanks everyone,
Daniel