[PATCH v5 0/5] perf util: bpf perf improvements

[PATCH v5 0/5] perf util: bpf perf improvements

[Song Liu]

This patches set improves bpf_perf (perf-stat --bpf-counter) by
 1) exposing key definitions to a libperf header;
 2) adding compatibility check for perf_attr_map;
 3) introducing config stat.bpf-counter-events.
 4) introducing 'b' modify to event parser.
 5) add bpf_counter_ops->disable()

Changes v4 => v5:
1. Add bpf_counter_ops->disable(). (Namhyung, Jiri)

Changes v3 => v4:
1. Improve the logic that decides when to skip read_affinity_counters().
   (Jiri)
2. Clean up a condition in bpf_counters.c:read_counters(). (Jiri)

Changes v2 => v3:
1. Add 'b' modifier. (Jiri)
2. Allow configuring stat.bpf-counter-events with any event name (instead
  of limiting to hardware events). (Jiri)

Changes v1 => v2:
1. Separte 2/3 from 1/3. (Jiri)
2. Rename bperf.h to bpf_perf.h. (Jiri)
3. Other small fixes/optimizations. (Jiri)

Song Liu (5):
  perf util: move bpf_perf definitions to a libperf header
  perf bpf: check perf_attr_map is compatible with the perf binary
  perf-stat: introduce config stat.bpf-counter-events
  perf-stat: introduce ':b' modifier
  perf-stat: introduce bpf_counter_ops->disable()

 tools/lib/perf/include/perf/bpf_perf.h | 31 +++++++++++
 tools/perf/Documentation/perf-stat.txt |  2 +
 tools/perf/builtin-stat.c              | 42 +++++++++------
 tools/perf/util/bpf_counter.c          | 75 +++++++++++++++++---------
 tools/perf/util/bpf_counter.h          |  7 +++
 tools/perf/util/config.c               |  4 ++
 tools/perf/util/evlist.c               |  4 ++
 tools/perf/util/evsel.c                | 22 ++++++++
 tools/perf/util/evsel.h                |  9 ++++
 tools/perf/util/parse-events.c         |  8 ++-
 tools/perf/util/parse-events.l         |  2 +-
 tools/perf/util/target.h               |  5 --
 12 files changed, 162 insertions(+), 49 deletions(-)
 create mode 100644 tools/lib/perf/include/perf/bpf_perf.h

--
2.30.2

[PATCH v5 1/5] perf util: move bpf_perf definitions to a libperf header

[Song Liu]

By following the same protocol, other tools can share hardware PMCs with
perf. Move perf_event_attr_map_entry and BPF_PERF_DEFAULT_ATTR_MAP_PATH to
bpf_perf.h for other tools to use.

Signed-off-by: Song Liu <song@kernel.org>
---
 tools/lib/perf/include/perf/bpf_perf.h | 31 ++++++++++++++++++++++++++
 tools/perf/util/bpf_counter.c          | 27 +++-------------------
 2 files changed, 34 insertions(+), 24 deletions(-)
 create mode 100644 tools/lib/perf/include/perf/bpf_perf.h

diff --git a/tools/lib/perf/include/perf/bpf_perf.h b/tools/lib/perf/include/perf/bpf_perf.h
new file mode 100644
index 0000000000000..e7cf6ba7b674b
--- /dev/null
+++ b/tools/lib/perf/include/perf/bpf_perf.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
+#ifndef __LIBPERF_BPF_PERF_H
+#define __LIBPERF_BPF_PERF_H
+
+#include <linux/types.h>  /* for __u32 */
+
+/*
+ * bpf_perf uses a hashmap, the attr_map, to track all the leader programs.
+ * The hashmap is pinned in bpffs. flock() on this file is used to ensure
+ * no concurrent access to the attr_map.  The key of attr_map is struct
+ * perf_event_attr, and the value is struct perf_event_attr_map_entry.
+ *
+ * struct perf_event_attr_map_entry contains two __u32 IDs, bpf_link of the
+ * leader prog, and the diff_map. Each perf-stat session holds a reference
+ * to the bpf_link to make sure the leader prog is attached to sched_switch
+ * tracepoint.
+ *
+ * Since the hashmap only contains IDs of the bpf_link and diff_map, it
+ * does not hold any references to the leader program. Once all perf-stat
+ * sessions of these events exit, the leader prog, its maps, and the
+ * perf_events will be freed.
+ */
+struct perf_event_attr_map_entry {
+	__u32 link_id;
+	__u32 diff_map_id;
+};
+
+/* default attr_map name */
+#define BPF_PERF_DEFAULT_ATTR_MAP_PATH "perf_attr_map"
+
+#endif /* __LIBPERF_BPF_PERF_H */
diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
index 81d1df3c4ec0e..be484ddbbd5be 100644
--- a/tools/perf/util/bpf_counter.c
+++ b/tools/perf/util/bpf_counter.c
@@ -14,6 +14,7 @@
 #include <bpf/btf.h>
 #include <bpf/libbpf.h>
 #include <api/fs/fs.h>
+#include <perf/bpf_perf.h>
 
 #include "bpf_counter.h"
 #include "counts.h"
@@ -29,28 +30,6 @@
 #include "bpf_skel/bperf_leader.skel.h"
 #include "bpf_skel/bperf_follower.skel.h"
 
-/*
- * bperf uses a hashmap, the attr_map, to track all the leader programs.
- * The hashmap is pinned in bpffs. flock() on this file is used to ensure
- * no concurrent access to the attr_map.  The key of attr_map is struct
- * perf_event_attr, and the value is struct perf_event_attr_map_entry.
- *
- * struct perf_event_attr_map_entry contains two __u32 IDs, bpf_link of the
- * leader prog, and the diff_map. Each perf-stat session holds a reference
- * to the bpf_link to make sure the leader prog is attached to sched_switch
- * tracepoint.
- *
- * Since the hashmap only contains IDs of the bpf_link and diff_map, it
- * does not hold any references to the leader program. Once all perf-stat
- * sessions of these events exit, the leader prog, its maps, and the
- * perf_events will be freed.
- */
-struct perf_event_attr_map_entry {
-	__u32 link_id;
-	__u32 diff_map_id;
-};
-
-#define DEFAULT_ATTR_MAP_PATH "fs/bpf/perf_attr_map"
 #define ATTR_MAP_SIZE 16
 
 static inline void *u64_to_ptr(__u64 ptr)
@@ -341,8 +320,8 @@ static int bperf_lock_attr_map(struct target *target)
 	if (target->attr_map) {
 		scnprintf(path, PATH_MAX, "%s", target->attr_map);
 	} else {
-		scnprintf(path, PATH_MAX, "%s/%s", sysfs__mountpoint(),
-			  DEFAULT_ATTR_MAP_PATH);
+		scnprintf(path, PATH_MAX, "%s/fs/bpf/%s", sysfs__mountpoint(),
+			  BPF_PERF_DEFAULT_ATTR_MAP_PATH);
 	}
 
 	if (access(path, F_OK)) {
-- 
2.30.2

[PATCH v5 2/5] perf bpf: check perf_attr_map is compatible with the perf binary

[Song Liu]

perf_attr_map could be shared among different version of perf binary. Add
bperf_attr_map_compatible() to check whether the existing attr_map is
compatible with current perf binary.

Signed-off-by: Song Liu <song@kernel.org>
---
 tools/perf/util/bpf_counter.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
index be484ddbbd5be..5de991ab46af9 100644
--- a/tools/perf/util/bpf_counter.c
+++ b/tools/perf/util/bpf_counter.c
@@ -312,6 +312,20 @@ static __u32 bpf_map_get_id(int fd)
 	return map_info.id;
 }
 
+static bool bperf_attr_map_compatible(int attr_map_fd)
+{
+	struct bpf_map_info map_info = {0};
+	__u32 map_info_len = sizeof(map_info);
+	int err;
+
+	err = bpf_obj_get_info_by_fd(attr_map_fd, &map_info, &map_info_len);
+
+	if (err)
+		return false;
+	return (map_info.key_size == sizeof(struct perf_event_attr)) &&
+		(map_info.value_size == sizeof(struct perf_event_attr_map_entry));
+}
+
 static int bperf_lock_attr_map(struct target *target)
 {
 	char path[PATH_MAX];
@@ -346,6 +360,11 @@ static int bperf_lock_attr_map(struct target *target)
 			return -1;
 	}
 
+	if (!bperf_attr_map_compatible(map_fd)) {
+		close(map_fd);
+		return -1;
+
+	}
 	err = flock(map_fd, LOCK_EX);
 	if (err) {
 		close(map_fd);
-- 
2.30.2

[PATCH v5 3/5] perf-stat: introduce config stat.bpf-counter-events

[Song Liu]

Currently, to use BPF to aggregate perf event counters, the user uses
--bpf-counters option. Enable "use bpf by default" events with a config
option, stat.bpf-counter-events. Events with name in the option will use
BPF.

This also enables mixed BPF event and regular event in the same sesssion.
For example:

   perf config stat.bpf-counter-events=instructions
   perf stat -e instructions,cs

The second command will use BPF for "instructions" but not "cs".

Signed-off-by: Song Liu <song@kernel.org>
---
 tools/perf/Documentation/perf-stat.txt |  2 ++
 tools/perf/builtin-stat.c              | 42 +++++++++++++++-----------
 tools/perf/util/bpf_counter.c          |  3 +-
 tools/perf/util/config.c               |  4 +++
 tools/perf/util/evsel.c                | 22 ++++++++++++++
 tools/perf/util/evsel.h                |  8 +++++
 tools/perf/util/target.h               |  5 ---
 7 files changed, 63 insertions(+), 23 deletions(-)

diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
index 6ec5960b08c3d..f10e24da23e90 100644
--- a/tools/perf/Documentation/perf-stat.txt
+++ b/tools/perf/Documentation/perf-stat.txt
@@ -97,6 +97,8 @@ report::
 	Use BPF programs to aggregate readings from perf_events.  This
 	allows multiple perf-stat sessions that are counting the same metric (cycles,
 	instructions, etc.) to share hardware counters.
+	To use BPF programs on common events by default, use
+	"perf config stat.bpf-counter-events=<list_of_events>".
 
 --bpf-attr-map::
 	With option "--bpf-counters", different perf-stat sessions share
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 2a2c15cac80a3..8729eed30b668 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -160,6 +160,7 @@ static const char *smi_cost_attrs = {
 };
 
 static struct evlist	*evsel_list;
+static bool all_counters_use_bpf = true;
 
 static struct target target = {
 	.uid	= UINT_MAX,
@@ -399,6 +400,9 @@ static int read_affinity_counters(struct timespec *rs)
 	struct affinity affinity;
 	int i, ncpus, cpu;
 
+	if (all_counters_use_bpf)
+		return 0;
+
 	if (affinity__setup(&affinity) < 0)
 		return -1;
 
@@ -413,6 +417,8 @@ static int read_affinity_counters(struct timespec *rs)
 		evlist__for_each_entry(evsel_list, counter) {
 			if (evsel__cpu_iter_skip(counter, cpu))
 				continue;
+			if (evsel__is_bpf(counter))
+				continue;
 			if (!counter->err) {
 				counter->err = read_counter_cpu(counter, rs,
 								counter->cpu_iter - 1);
@@ -429,6 +435,9 @@ static int read_bpf_map_counters(void)
 	int err;
 
 	evlist__for_each_entry(evsel_list, counter) {
+		if (!evsel__is_bpf(counter))
+			continue;
+
 		err = bpf_counter__read(counter);
 		if (err)
 			return err;
@@ -439,14 +448,10 @@ static int read_bpf_map_counters(void)
 static void read_counters(struct timespec *rs)
 {
 	struct evsel *counter;
-	int err;
 
 	if (!stat_config.stop_read_counter) {
-		if (target__has_bpf(&target))
-			err = read_bpf_map_counters();
-		else
-			err = read_affinity_counters(rs);
-		if (err < 0)
+		if (read_bpf_map_counters() ||
+		    read_affinity_counters(rs))
 			return;
 	}
 
@@ -535,12 +540,13 @@ static int enable_counters(void)
 	struct evsel *evsel;
 	int err;
 
-	if (target__has_bpf(&target)) {
-		evlist__for_each_entry(evsel_list, evsel) {
-			err = bpf_counter__enable(evsel);
-			if (err)
-				return err;
-		}
+	evlist__for_each_entry(evsel_list, evsel) {
+		if (!evsel__is_bpf(evsel))
+			continue;
+
+		err = bpf_counter__enable(evsel);
+		if (err)
+			return err;
 	}
 
 	if (stat_config.initial_delay < 0) {
@@ -784,11 +790,11 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	if (affinity__setup(&affinity) < 0)
 		return -1;
 
-	if (target__has_bpf(&target)) {
-		evlist__for_each_entry(evsel_list, counter) {
-			if (bpf_counter__load(counter, &target))
-				return -1;
-		}
+	evlist__for_each_entry(evsel_list, counter) {
+		if (bpf_counter__load(counter, &target))
+			return -1;
+		if (!evsel__is_bpf(counter))
+			all_counters_use_bpf = false;
 	}
 
 	evlist__for_each_cpu (evsel_list, i, cpu) {
@@ -805,6 +811,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 				continue;
 			if (counter->reset_group || counter->errored)
 				continue;
+			if (evsel__is_bpf(counter))
+				continue;
 try_again:
 			if (create_perf_stat_counter(counter, &stat_config, &target,
 						     counter->cpu_iter - 1) < 0) {
diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
index 5de991ab46af9..33b1888103dfa 100644
--- a/tools/perf/util/bpf_counter.c
+++ b/tools/perf/util/bpf_counter.c
@@ -790,7 +790,8 @@ int bpf_counter__load(struct evsel *evsel, struct target *target)
 {
 	if (target->bpf_str)
 		evsel->bpf_counter_ops = &bpf_program_profiler_ops;
-	else if (target->use_bpf)
+	else if (target->use_bpf ||
+		 evsel__match_bpf_counter_events(evsel->name))
 		evsel->bpf_counter_ops = &bperf_ops;
 
 	if (evsel->bpf_counter_ops)
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 6bcb5ef221f8c..63d472b336de2 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -18,6 +18,7 @@
 #include "util/hist.h"  /* perf_hist_config */
 #include "util/llvm-utils.h"   /* perf_llvm_config */
 #include "util/stat.h"  /* perf_stat__set_big_num */
+#include "util/evsel.h"  /* evsel__hw_names, evsel__use_bpf_counters */
 #include "build-id.h"
 #include "debug.h"
 #include "config.h"
@@ -460,6 +461,9 @@ static int perf_stat_config(const char *var, const char *value)
 	if (!strcmp(var, "stat.no-csv-summary"))
 		perf_stat__set_no_csv_summary(perf_config_bool(var, value));
 
+	if (!strcmp(var, "stat.bpf-counter-events"))
+		evsel__bpf_counter_events = strdup(value);
+
 	/* Add other config variables here. */
 	return 0;
 }
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 2d2614eeaa20e..080ddcfefbcd2 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -492,6 +492,28 @@ const char *evsel__hw_names[PERF_COUNT_HW_MAX] = {
 	"ref-cycles",
 };
 
+char *evsel__bpf_counter_events;
+
+bool evsel__match_bpf_counter_events(const char *name)
+{
+	int name_len;
+	bool match;
+	char *ptr;
+
+	if (!evsel__bpf_counter_events)
+		return false;
+
+	ptr = strstr(evsel__bpf_counter_events, name);
+	name_len = strlen(name);
+
+	/* check name matches a full token in evsel__bpf_counter_events */
+	match = (ptr != NULL) &&
+		((ptr == evsel__bpf_counter_events) || (*(ptr - 1) == ',')) &&
+		((*(ptr + name_len) == ',') || (*(ptr + name_len) == '\0'));
+
+	return match;
+}
+
 static const char *__evsel__hw_name(u64 config)
 {
 	if (config < PERF_COUNT_HW_MAX && evsel__hw_names[config])
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index eccc4fd5b3eb4..ce4b629d659c2 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -239,6 +239,11 @@ void evsel__calc_id_pos(struct evsel *evsel);
 
 bool evsel__is_cache_op_valid(u8 type, u8 op);
 
+static inline bool evsel__is_bpf(struct evsel *evsel)
+{
+	return evsel->bpf_counter_ops != NULL;
+}
+
 #define EVSEL__MAX_ALIASES 8
 
 extern const char *evsel__hw_cache[PERF_COUNT_HW_CACHE_MAX][EVSEL__MAX_ALIASES];
@@ -246,6 +251,9 @@ extern const char *evsel__hw_cache_op[PERF_COUNT_HW_CACHE_OP_MAX][EVSEL__MAX_ALI
 extern const char *evsel__hw_cache_result[PERF_COUNT_HW_CACHE_RESULT_MAX][EVSEL__MAX_ALIASES];
 extern const char *evsel__hw_names[PERF_COUNT_HW_MAX];
 extern const char *evsel__sw_names[PERF_COUNT_SW_MAX];
+extern char *evsel__bpf_counter_events;
+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);
 
diff --git a/tools/perf/util/target.h b/tools/perf/util/target.h
index 1bce3eb28ef25..4ff56217f2a65 100644
--- a/tools/perf/util/target.h
+++ b/tools/perf/util/target.h
@@ -66,11 +66,6 @@ static inline bool target__has_cpu(struct target *target)
 	return target->system_wide || target->cpu_list;
 }
 
-static inline bool target__has_bpf(struct target *target)
-{
-	return target->bpf_str || target->use_bpf;
-}
-
 static inline bool target__none(struct target *target)
 {
 	return !target__has_task(target) && !target__has_cpu(target);
-- 
2.30.2

[PATCH v5 4/5] perf-stat: introduce ':b' modifier

[Song Liu]

Introduce 'b' modifier to event parser, which means use BPF program to
manage this event. This is the same as --bpf-counters option, but only
applies to this event. For example,

  perf stat -e cycles:b,cs               # use bpf for cycles, but not cs
  perf stat -e cycles,cs --bpf-counters  # use bpf for both cycles and cs

Suggested-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Song Liu <song@kernel.org>
---
 tools/perf/util/bpf_counter.c  | 2 +-
 tools/perf/util/evsel.h        | 1 +
 tools/perf/util/parse-events.c | 8 +++++++-
 tools/perf/util/parse-events.l | 2 +-
 4 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
index 33b1888103dfa..f179f57430253 100644
--- a/tools/perf/util/bpf_counter.c
+++ b/tools/perf/util/bpf_counter.c
@@ -790,7 +790,7 @@ int bpf_counter__load(struct evsel *evsel, struct target *target)
 {
 	if (target->bpf_str)
 		evsel->bpf_counter_ops = &bpf_program_profiler_ops;
-	else if (target->use_bpf ||
+	else if (target->use_bpf || evsel->bpf_counter ||
 		 evsel__match_bpf_counter_events(evsel->name))
 		evsel->bpf_counter_ops = &bperf_ops;
 
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index ce4b629d659c2..8f66cdcb338d0 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -82,6 +82,7 @@ struct evsel {
 		bool			auto_merge_stats;
 		bool			collect_stat;
 		bool			weak_group;
+		bool			bpf_counter;
 		int			bpf_fd;
 		struct bpf_object	*bpf_obj;
 	};
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 8123d218ad17c..46ebd269a98d1 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1804,6 +1804,7 @@ struct event_modifier {
 	int pinned;
 	int weak;
 	int exclusive;
+	int bpf_counter;
 };
 
 static int get_event_modifier(struct event_modifier *mod, char *str,
@@ -1824,6 +1825,7 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
 	int exclude = eu | ek | eh;
 	int exclude_GH = evsel ? evsel->exclude_GH : 0;
 	int weak = 0;
+	int bpf_counter = 0;
 
 	memset(mod, 0, sizeof(*mod));
 
@@ -1867,6 +1869,8 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
 			exclusive = 1;
 		} else if (*str == 'W') {
 			weak = 1;
+		} else if (*str == 'b') {
+			bpf_counter = 1;
 		} else
 			break;
 
@@ -1898,6 +1902,7 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
 	mod->sample_read = sample_read;
 	mod->pinned = pinned;
 	mod->weak = weak;
+	mod->bpf_counter = bpf_counter;
 	mod->exclusive = exclusive;
 
 	return 0;
@@ -1912,7 +1917,7 @@ static int check_modifier(char *str)
 	char *p = str;
 
 	/* The sizeof includes 0 byte as well. */
-	if (strlen(str) > (sizeof("ukhGHpppPSDIWe") - 1))
+	if (strlen(str) > (sizeof("ukhGHpppPSDIWeb") - 1))
 		return -1;
 
 	while (*p) {
@@ -1953,6 +1958,7 @@ int parse_events__modifier_event(struct list_head *list, char *str, bool add)
 		evsel->sample_read         = mod.sample_read;
 		evsel->precise_max         = mod.precise_max;
 		evsel->weak_group	   = mod.weak;
+		evsel->bpf_counter	   = mod.bpf_counter;
 
 		if (evsel__is_group_leader(evsel)) {
 			evsel->core.attr.pinned = mod.pinned;
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 0b36285a9435d..fb8646cc3e834 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -210,7 +210,7 @@ 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_*?\.:]+)?
 /* If you add a modifier you need to update check_modifier() */
-modifier_event	[ukhpPGHSDIWe]+
+modifier_event	[ukhpPGHSDIWeb]+
 modifier_bp	[rwx]{1,3}
 
 %%
-- 
2.30.2

[PATCH v5 5/5] perf-stat: introduce bpf_counter_ops->disable()

[Song Liu]

Introduce bpf_counter_ops->disable(), which is used stop counting the
event.

Signed-off-by: Song Liu <song@kernel.org>
---
 tools/perf/util/bpf_counter.c | 26 ++++++++++++++++++++++++++
 tools/perf/util/bpf_counter.h |  7 +++++++
 tools/perf/util/evlist.c      |  4 ++++
 3 files changed, 37 insertions(+)

diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
index f179f57430253..ddb52f748c8e8 100644
--- a/tools/perf/util/bpf_counter.c
+++ b/tools/perf/util/bpf_counter.c
@@ -215,6 +215,17 @@ static int bpf_program_profiler__enable(struct evsel *evsel)
 	return 0;
 }
 
+static int bpf_program_profiler__disable(struct evsel *evsel)
+{
+	struct bpf_counter *counter;
+
+	list_for_each_entry(counter, &evsel->bpf_counter_list, list) {
+		assert(counter->skel != NULL);
+		bpf_prog_profiler_bpf__detach(counter->skel);
+	}
+	return 0;
+}
+
 static int bpf_program_profiler__read(struct evsel *evsel)
 {
 	// perf_cpu_map uses /sys/devices/system/cpu/online
@@ -280,6 +291,7 @@ static int bpf_program_profiler__install_pe(struct evsel *evsel, int cpu,
 struct bpf_counter_ops bpf_program_profiler_ops = {
 	.load       = bpf_program_profiler__load,
 	.enable	    = bpf_program_profiler__enable,
+	.disable    = bpf_program_profiler__disable,
 	.read       = bpf_program_profiler__read,
 	.destroy    = bpf_program_profiler__destroy,
 	.install_pe = bpf_program_profiler__install_pe,
@@ -627,6 +639,12 @@ static int bperf__enable(struct evsel *evsel)
 	return 0;
 }
 
+static int bperf__disable(struct evsel *evsel)
+{
+	evsel->follower_skel->bss->enabled = 0;
+	return 0;
+}
+
 static int bperf__read(struct evsel *evsel)
 {
 	struct bperf_follower_bpf *skel = evsel->follower_skel;
@@ -768,6 +786,7 @@ static int bperf__destroy(struct evsel *evsel)
 struct bpf_counter_ops bperf_ops = {
 	.load       = bperf__load,
 	.enable     = bperf__enable,
+	.disable    = bperf__disable,
 	.read       = bperf__read,
 	.install_pe = bperf__install_pe,
 	.destroy    = bperf__destroy,
@@ -806,6 +825,13 @@ int bpf_counter__enable(struct evsel *evsel)
 	return evsel->bpf_counter_ops->enable(evsel);
 }
 
+int bpf_counter__disable(struct evsel *evsel)
+{
+	if (bpf_counter_skip(evsel))
+		return 0;
+	return evsel->bpf_counter_ops->disable(evsel);
+}
+
 int bpf_counter__read(struct evsel *evsel)
 {
 	if (bpf_counter_skip(evsel))
diff --git a/tools/perf/util/bpf_counter.h b/tools/perf/util/bpf_counter.h
index cb9c532e0a079..d6d907c3dcf92 100644
--- a/tools/perf/util/bpf_counter.h
+++ b/tools/perf/util/bpf_counter.h
@@ -18,6 +18,7 @@ typedef int (*bpf_counter_evsel_install_pe_op)(struct evsel *evsel,
 struct bpf_counter_ops {
 	bpf_counter_evsel_target_op load;
 	bpf_counter_evsel_op enable;
+	bpf_counter_evsel_op disable;
 	bpf_counter_evsel_op read;
 	bpf_counter_evsel_op destroy;
 	bpf_counter_evsel_install_pe_op install_pe;
@@ -32,6 +33,7 @@ struct bpf_counter {
 
 int bpf_counter__load(struct evsel *evsel, struct target *target);
 int bpf_counter__enable(struct evsel *evsel);
+int bpf_counter__disable(struct evsel *evsel);
 int bpf_counter__read(struct evsel *evsel);
 void bpf_counter__destroy(struct evsel *evsel);
 int bpf_counter__install_pe(struct evsel *evsel, int cpu, int fd);
@@ -51,6 +53,11 @@ static inline int bpf_counter__enable(struct evsel *evsel __maybe_unused)
 	return 0;
 }
 
+static inline int bpf_counter__disable(struct evsel *evsel __maybe_unused)
+{
+	return 0;
+}
+
 static inline int bpf_counter__read(struct evsel *evsel __maybe_unused)
 {
 	return -EAGAIN;
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index d29a8a118973c..e71041c890102 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -17,6 +17,7 @@
 #include "evsel.h"
 #include "debug.h"
 #include "units.h"
+#include "bpf_counter.h"
 #include <internal/lib.h> // page_size
 #include "affinity.h"
 #include "../perf.h"
@@ -421,6 +422,9 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
 	if (affinity__setup(&affinity) < 0)
 		return;
 
+	evlist__for_each_entry(evlist, pos)
+		bpf_counter__disable(pos);
+
 	/* Disable 'immediate' events last */
 	for (imm = 0; imm <= 1; imm++) {
 		evlist__for_each_cpu(evlist, i, cpu) {
-- 
2.30.2

Re: [PATCH v5 5/5] perf-stat: introduce bpf_counter_ops->disable()

[Jiri Olsa]

On Sun, Apr 25, 2021 at 02:43:33PM -0700, Song Liu wrote:

SNIP

> +static inline int bpf_counter__disable(struct evsel *evsel __maybe_unused)
> +{
> +	return 0;
> +}
> +
>  static inline int bpf_counter__read(struct evsel *evsel __maybe_unused)
>  {
>  	return -EAGAIN;
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index d29a8a118973c..e71041c890102 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -17,6 +17,7 @@
>  #include "evsel.h"
>  #include "debug.h"
>  #include "units.h"
> +#include "bpf_counter.h"
>  #include <internal/lib.h> // page_size
>  #include "affinity.h"
>  #include "../perf.h"
> @@ -421,6 +422,9 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
>  	if (affinity__setup(&affinity) < 0)
>  		return;
>  
> +	evlist__for_each_entry(evlist, pos)
> +		bpf_counter__disable(pos);

I was wondering why you don't check evsel__is_bpf like
for the enable case.. and realized that we don't skip
bpf evsels in __evlist__enable and __evlist__disable
like we do in read_affinity_counters

so I guess there's extra affinity setup and bunch of
wrong ioctls being called?

jirka


> +
>  	/* Disable 'immediate' events last */
>  	for (imm = 0; imm <= 1; imm++) {
>  		evlist__for_each_cpu(evlist, i, cpu) {
> -- 
> 2.30.2
> 

Re: [PATCH v5 5/5] perf-stat: introduce bpf_counter_ops->disable()

[Song Liu]

> On Apr 26, 2021, at 2:27 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> 
> On Sun, Apr 25, 2021 at 02:43:33PM -0700, Song Liu wrote:
> 
> SNIP
> 
>> +static inline int bpf_counter__disable(struct evsel *evsel __maybe_unused)
>> +{
>> +	return 0;
>> +}
>> +
>> static inline int bpf_counter__read(struct evsel *evsel __maybe_unused)
>> {
>> 	return -EAGAIN;
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index d29a8a118973c..e71041c890102 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -17,6 +17,7 @@
>> #include "evsel.h"
>> #include "debug.h"
>> #include "units.h"
>> +#include "bpf_counter.h"
>> #include <internal/lib.h> // page_size
>> #include "affinity.h"
>> #include "../perf.h"
>> @@ -421,6 +422,9 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
>> 	if (affinity__setup(&affinity) < 0)
>> 		return;
>> 
>> +	evlist__for_each_entry(evlist, pos)
>> +		bpf_counter__disable(pos);
> 
> I was wondering why you don't check evsel__is_bpf like
> for the enable case.. and realized that we don't skip
> bpf evsels in __evlist__enable and __evlist__disable
> like we do in read_affinity_counters
> 
> so I guess there's extra affinity setup and bunch of
> wrong ioctls being called?

We actually didn't do wrong ioctls because the following check:

       if (... || !pos->core.fd)
                continue;

in __evlist__enable and __evlist__disable. That we don't allocate 
core.fd for is_bpf events. 

It is probably good to be more safe with an extra check of 
evsel__is_bpf(). But it is not required with current code. 

Thanks,
Song

[...]

Re: [PATCH v5 5/5] perf-stat: introduce bpf_counter_ops->disable()

[Jiri Olsa]

On Mon, Apr 26, 2021 at 10:18:57PM +0000, Song Liu wrote:
> 
> 
> > On Apr 26, 2021, at 2:27 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> > 
> > On Sun, Apr 25, 2021 at 02:43:33PM -0700, Song Liu wrote:
> > 
> > SNIP
> > 
> >> +static inline int bpf_counter__disable(struct evsel *evsel __maybe_unused)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> static inline int bpf_counter__read(struct evsel *evsel __maybe_unused)
> >> {
> >> 	return -EAGAIN;
> >> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >> index d29a8a118973c..e71041c890102 100644
> >> --- a/tools/perf/util/evlist.c
> >> +++ b/tools/perf/util/evlist.c
> >> @@ -17,6 +17,7 @@
> >> #include "evsel.h"
> >> #include "debug.h"
> >> #include "units.h"
> >> +#include "bpf_counter.h"
> >> #include <internal/lib.h> // page_size
> >> #include "affinity.h"
> >> #include "../perf.h"
> >> @@ -421,6 +422,9 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
> >> 	if (affinity__setup(&affinity) < 0)
> >> 		return;
> >> 
> >> +	evlist__for_each_entry(evlist, pos)
> >> +		bpf_counter__disable(pos);
> > 
> > I was wondering why you don't check evsel__is_bpf like
> > for the enable case.. and realized that we don't skip
> > bpf evsels in __evlist__enable and __evlist__disable
> > like we do in read_affinity_counters
> > 
> > so I guess there's extra affinity setup and bunch of
> > wrong ioctls being called?
> 
> We actually didn't do wrong ioctls because the following check:
> 
>        if (... || !pos->core.fd)
>                 continue;
> 
> in __evlist__enable and __evlist__disable. That we don't allocate 
> core.fd for is_bpf events. 
> 
> It is probably good to be more safe with an extra check of 
> evsel__is_bpf(). But it is not required with current code. 

hum, but it will do all the affinity setup no? for no reason,
if there's no non-bpb event

jirka

> 
> Thanks,
> Song
> 
> [...]
> 

Re: [PATCH v5 5/5] perf-stat: introduce bpf_counter_ops->disable()

[Arnaldo Carvalho de Melo]

Em Sun, Apr 25, 2021 at 02:43:33PM -0700, Song Liu escreveu:
> Introduce bpf_counter_ops->disable(), which is used stop counting the
> event.

[acme@five perf]$ perf test -v python
Couldn't bump rlimit(MEMLOCK), failures may take place when creating BPF maps, etc
19: 'import perf' in python                                         :
--- start ---
test child forked, pid 1497924
python usage test: "echo "import sys ; sys.path.append('/tmp/build/perf/python'); import perf" | '/usr/bin/python3' "
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: /tmp/build/perf/python/perf.cpython-39-x86_64-linux-gnu.so: undefined symbol: bpf_counter__disable
test child finished with -1
---- end ----
'import perf' in python: FAILED!
[acme@five perf]$

I'll fix this up in my local tree, if you need to respin, please pick
patches from tmp.perf/core, will refresh it later today.

- Arnaldo
 
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  tools/perf/util/bpf_counter.c | 26 ++++++++++++++++++++++++++
>  tools/perf/util/bpf_counter.h |  7 +++++++
>  tools/perf/util/evlist.c      |  4 ++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
> index f179f57430253..ddb52f748c8e8 100644
> --- a/tools/perf/util/bpf_counter.c
> +++ b/tools/perf/util/bpf_counter.c
> @@ -215,6 +215,17 @@ static int bpf_program_profiler__enable(struct evsel *evsel)
>  	return 0;
>  }
>  
> +static int bpf_program_profiler__disable(struct evsel *evsel)
> +{
> +	struct bpf_counter *counter;
> +
> +	list_for_each_entry(counter, &evsel->bpf_counter_list, list) {
> +		assert(counter->skel != NULL);
> +		bpf_prog_profiler_bpf__detach(counter->skel);
> +	}
> +	return 0;
> +}
> +
>  static int bpf_program_profiler__read(struct evsel *evsel)
>  {
>  	// perf_cpu_map uses /sys/devices/system/cpu/online
> @@ -280,6 +291,7 @@ static int bpf_program_profiler__install_pe(struct evsel *evsel, int cpu,
>  struct bpf_counter_ops bpf_program_profiler_ops = {
>  	.load       = bpf_program_profiler__load,
>  	.enable	    = bpf_program_profiler__enable,
> +	.disable    = bpf_program_profiler__disable,
>  	.read       = bpf_program_profiler__read,
>  	.destroy    = bpf_program_profiler__destroy,
>  	.install_pe = bpf_program_profiler__install_pe,
> @@ -627,6 +639,12 @@ static int bperf__enable(struct evsel *evsel)
>  	return 0;
>  }
>  
> +static int bperf__disable(struct evsel *evsel)
> +{
> +	evsel->follower_skel->bss->enabled = 0;
> +	return 0;
> +}
> +
>  static int bperf__read(struct evsel *evsel)
>  {
>  	struct bperf_follower_bpf *skel = evsel->follower_skel;
> @@ -768,6 +786,7 @@ static int bperf__destroy(struct evsel *evsel)
>  struct bpf_counter_ops bperf_ops = {
>  	.load       = bperf__load,
>  	.enable     = bperf__enable,
> +	.disable    = bperf__disable,
>  	.read       = bperf__read,
>  	.install_pe = bperf__install_pe,
>  	.destroy    = bperf__destroy,
> @@ -806,6 +825,13 @@ int bpf_counter__enable(struct evsel *evsel)
>  	return evsel->bpf_counter_ops->enable(evsel);
>  }
>  
> +int bpf_counter__disable(struct evsel *evsel)
> +{
> +	if (bpf_counter_skip(evsel))
> +		return 0;
> +	return evsel->bpf_counter_ops->disable(evsel);
> +}
> +
>  int bpf_counter__read(struct evsel *evsel)
>  {
>  	if (bpf_counter_skip(evsel))
> diff --git a/tools/perf/util/bpf_counter.h b/tools/perf/util/bpf_counter.h
> index cb9c532e0a079..d6d907c3dcf92 100644
> --- a/tools/perf/util/bpf_counter.h
> +++ b/tools/perf/util/bpf_counter.h
> @@ -18,6 +18,7 @@ typedef int (*bpf_counter_evsel_install_pe_op)(struct evsel *evsel,
>  struct bpf_counter_ops {
>  	bpf_counter_evsel_target_op load;
>  	bpf_counter_evsel_op enable;
> +	bpf_counter_evsel_op disable;
>  	bpf_counter_evsel_op read;
>  	bpf_counter_evsel_op destroy;
>  	bpf_counter_evsel_install_pe_op install_pe;
> @@ -32,6 +33,7 @@ struct bpf_counter {
>  
>  int bpf_counter__load(struct evsel *evsel, struct target *target);
>  int bpf_counter__enable(struct evsel *evsel);
> +int bpf_counter__disable(struct evsel *evsel);
>  int bpf_counter__read(struct evsel *evsel);
>  void bpf_counter__destroy(struct evsel *evsel);
>  int bpf_counter__install_pe(struct evsel *evsel, int cpu, int fd);
> @@ -51,6 +53,11 @@ static inline int bpf_counter__enable(struct evsel *evsel __maybe_unused)
>  	return 0;
>  }
>  
> +static inline int bpf_counter__disable(struct evsel *evsel __maybe_unused)
> +{
> +	return 0;
> +}
> +
>  static inline int bpf_counter__read(struct evsel *evsel __maybe_unused)
>  {
>  	return -EAGAIN;
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index d29a8a118973c..e71041c890102 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -17,6 +17,7 @@
>  #include "evsel.h"
>  #include "debug.h"
>  #include "units.h"
> +#include "bpf_counter.h"
>  #include <internal/lib.h> // page_size
>  #include "affinity.h"
>  #include "../perf.h"
> @@ -421,6 +422,9 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
>  	if (affinity__setup(&affinity) < 0)
>  		return;
>  
> +	evlist__for_each_entry(evlist, pos)
> +		bpf_counter__disable(pos);
> +
>  	/* Disable 'immediate' events last */
>  	for (imm = 0; imm <= 1; imm++) {
>  		evlist__for_each_cpu(evlist, i, cpu) {
> -- 
> 2.30.2
> 

-- 

- Arnaldo

Re: [PATCH v5 5/5] perf-stat: introduce bpf_counter_ops->disable()

[Song Liu]

> On Apr 27, 2021, at 5:33 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> 
> On Mon, Apr 26, 2021 at 10:18:57PM +0000, Song Liu wrote:
>> 
>> 
>>> On Apr 26, 2021, at 2:27 PM, Jiri Olsa <jolsa@redhat.com> wrote:
>>> 
>>> On Sun, Apr 25, 2021 at 02:43:33PM -0700, Song Liu wrote:
>>> 
>>> SNIP
>>> 
>>>> +static inline int bpf_counter__disable(struct evsel *evsel __maybe_unused)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> static inline int bpf_counter__read(struct evsel *evsel __maybe_unused)
>>>> {
>>>> 	return -EAGAIN;
>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>> index d29a8a118973c..e71041c890102 100644
>>>> --- a/tools/perf/util/evlist.c
>>>> +++ b/tools/perf/util/evlist.c
>>>> @@ -17,6 +17,7 @@
>>>> #include "evsel.h"
>>>> #include "debug.h"
>>>> #include "units.h"
>>>> +#include "bpf_counter.h"
>>>> #include <internal/lib.h> // page_size
>>>> #include "affinity.h"
>>>> #include "../perf.h"
>>>> @@ -421,6 +422,9 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
>>>> 	if (affinity__setup(&affinity) < 0)
>>>> 		return;
>>>> 
>>>> +	evlist__for_each_entry(evlist, pos)
>>>> +		bpf_counter__disable(pos);
>>> 
>>> I was wondering why you don't check evsel__is_bpf like
>>> for the enable case.. and realized that we don't skip
>>> bpf evsels in __evlist__enable and __evlist__disable
>>> like we do in read_affinity_counters
>>> 
>>> so I guess there's extra affinity setup and bunch of
>>> wrong ioctls being called?
>> 
>> We actually didn't do wrong ioctls because the following check:
>> 
>>       if (... || !pos->core.fd)
>>                continue;
>> 
>> in __evlist__enable and __evlist__disable. That we don't allocate 
>> core.fd for is_bpf events. 
>> 
>> It is probably good to be more safe with an extra check of 
>> evsel__is_bpf(). But it is not required with current code. 
> 
> hum, but it will do all the affinity setup no? for no reason,
> if there's no non-bpb event

Yes, it will do the affinity setup. Let me see how to get something
like all_counters_use_bpf here (or within builtin-stat.c).

Thanks,
Song

Re: [PATCH v5 5/5] perf-stat: introduce bpf_counter_ops->disable()

[Arnaldo Carvalho de Melo]

Em Tue, Apr 27, 2021 at 04:27:22PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sun, Apr 25, 2021 at 02:43:33PM -0700, Song Liu escreveu:
> > Introduce bpf_counter_ops->disable(), which is used stop counting the
> > event.
> 
> [acme@five perf]$ perf test -v python
> Couldn't bump rlimit(MEMLOCK), failures may take place when creating BPF maps, etc
> 19: 'import perf' in python                                         :
> --- start ---
> test child forked, pid 1497924
> python usage test: "echo "import sys ; sys.path.append('/tmp/build/perf/python'); import perf" | '/usr/bin/python3' "
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> ImportError: /tmp/build/perf/python/perf.cpython-39-x86_64-linux-gnu.so: undefined symbol: bpf_counter__disable
> test child finished with -1
> ---- end ----
> 'import perf' in python: FAILED!
> [acme@five perf]$
> 
> I'll fix this up in my local tree, if you need to respin, please pick
> patches from tmp.perf/core, will refresh it later today.

Added this:


diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 278abecb5bdfc0d2..27940edb161c2d8c 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -100,6 +100,11 @@ int bpf_counter__install_pe(struct evsel *evsel __maybe_unused, int cpu __maybe_
 	return 0;
 }
 
+int bpf_counter__disable(struct evsel *evsel __maybe_unused)
+{
+	return 0;
+}
+
 /*
  * Support debug printing even though util/debug.c is not linked.  That means
  * implementing 'verbose' and 'eprintf'.

Re: [PATCH v5 5/5] perf-stat: introduce bpf_counter_ops->disable()

[Song Liu]

> On Apr 27, 2021, at 12:30 PM, Song Liu <songliubraving@fb.com> wrote:
> 
> 
> 
>> On Apr 27, 2021, at 5:33 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>> 
>> On Mon, Apr 26, 2021 at 10:18:57PM +0000, Song Liu wrote:
>>> 
>>> 
>>>> On Apr 26, 2021, at 2:27 PM, Jiri Olsa <jolsa@redhat.com> wrote:
>>>> 
>>>> On Sun, Apr 25, 2021 at 02:43:33PM -0700, Song Liu wrote:
>>>> 
>>>> SNIP
>>>> 
>>>>> +static inline int bpf_counter__disable(struct evsel *evsel __maybe_unused)
>>>>> +{
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> static inline int bpf_counter__read(struct evsel *evsel __maybe_unused)
>>>>> {
>>>>> 	return -EAGAIN;
>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>>> index d29a8a118973c..e71041c890102 100644
>>>>> --- a/tools/perf/util/evlist.c
>>>>> +++ b/tools/perf/util/evlist.c
>>>>> @@ -17,6 +17,7 @@
>>>>> #include "evsel.h"
>>>>> #include "debug.h"
>>>>> #include "units.h"
>>>>> +#include "bpf_counter.h"
>>>>> #include <internal/lib.h> // page_size
>>>>> #include "affinity.h"
>>>>> #include "../perf.h"
>>>>> @@ -421,6 +422,9 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
>>>>> 	if (affinity__setup(&affinity) < 0)
>>>>> 		return;
>>>>> 
>>>>> +	evlist__for_each_entry(evlist, pos)
>>>>> +		bpf_counter__disable(pos);
>>>> 
>>>> I was wondering why you don't check evsel__is_bpf like
>>>> for the enable case.. and realized that we don't skip
>>>> bpf evsels in __evlist__enable and __evlist__disable
>>>> like we do in read_affinity_counters
>>>> 
>>>> so I guess there's extra affinity setup and bunch of
>>>> wrong ioctls being called?
>>> 
>>> We actually didn't do wrong ioctls because the following check:
>>> 
>>>      if (... || !pos->core.fd)
>>>               continue;
>>> 
>>> in __evlist__enable and __evlist__disable. That we don't allocate 
>>> core.fd for is_bpf events. 
>>> 
>>> It is probably good to be more safe with an extra check of 
>>> evsel__is_bpf(). But it is not required with current code. 
>> 
>> hum, but it will do all the affinity setup no? for no reason,
>> if there's no non-bpb event
> 
> Yes, it will do the affinity setup. Let me see how to get something
> like all_counters_use_bpf here (or within builtin-stat.c).
> 

Would something like the following work? It is not clean (skipping some 
useful logic in __evlist__[enable|disable]). But it seems to work in the
tests.

Thanks,
Song



From ecb75a1fa747ca5521bcda972840df1e97c09b11 Mon Sep 17 00:00:00 2001
From: Song Liu <song@kernel.org>
Date: Wed, 28 Apr 2021 17:41:28 -0700
Subject: [PATCH] perf-stat: skip evlist__[enable|disable] when all events uses
 BPF

When all events of a perf-stat session use BPF, it is not necessary to
call evlist__enable() and evlist__disable(). Skip them when
all_counters_use_bpf is true.

Signed-off-by: Song Liu <song@kernel.org>
---
 tools/perf/builtin-stat.c | 12 +++++++++---
 tools/perf/util/evlist.c  |  3 ---
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 5a830ae09418e..44459e0352fda 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -572,7 +572,8 @@ static int enable_counters(void)
         * - we have initial delay configured
         */
        if (!target__none(&target) || stat_config.initial_delay) {
-               evlist__enable(evsel_list);
+               if (!all_counters_use_bpf)
+                       evlist__enable(evsel_list);
                if (stat_config.initial_delay > 0)
                        pr_info(EVLIST_ENABLED_MSG);
        }
@@ -581,13 +582,18 @@ static int enable_counters(void)

 static void disable_counters(void)
 {
+       struct evsel *counter;
        /*
         * If we don't have tracee (attaching to task or cpu), counters may
         * still be running. To get accurate group ratios, we must stop groups
         * from counting before reading their constituent counters.
         */
-       if (!target__none(&target))
-               evlist__disable(evsel_list);
+       if (!target__none(&target)) {
+               evlist__for_each_entry(evsel_list, counter)
+                       bpf_counter__disable(counter);
+               if (!all_counters_use_bpf)
+                       evlist__disable(evsel_list);
+       }
 }

 static volatile int workload_exec_errno;
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 6e5c41528c7d0..6ea3e677dc1e7 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -425,9 +425,6 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
        if (affinity__setup(&affinity) < 0)
                return;

-       evlist__for_each_entry(evlist, pos)
-               bpf_counter__disable(pos);
-
        /* Disable 'immediate' events last */
        for (imm = 0; imm <= 1; imm++) {
                evlist__for_each_cpu(evlist, i, cpu) {
--
2.30.2

 

Re: [PATCH v5 5/5] perf-stat: introduce bpf_counter_ops->disable()

[Jiri Olsa]

On Thu, Apr 29, 2021 at 10:40:01PM +0000, Song Liu wrote:

SNIP

> >>>>> #include "../perf.h"
> >>>>> @@ -421,6 +422,9 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
> >>>>> 	if (affinity__setup(&affinity) < 0)
> >>>>> 		return;
> >>>>> 
> >>>>> +	evlist__for_each_entry(evlist, pos)
> >>>>> +		bpf_counter__disable(pos);
> >>>> 
> >>>> I was wondering why you don't check evsel__is_bpf like
> >>>> for the enable case.. and realized that we don't skip
> >>>> bpf evsels in __evlist__enable and __evlist__disable
> >>>> like we do in read_affinity_counters
> >>>> 
> >>>> so I guess there's extra affinity setup and bunch of
> >>>> wrong ioctls being called?
> >>> 
> >>> We actually didn't do wrong ioctls because the following check:
> >>> 
> >>>      if (... || !pos->core.fd)
> >>>               continue;
> >>> 
> >>> in __evlist__enable and __evlist__disable. That we don't allocate 
> >>> core.fd for is_bpf events. 
> >>> 
> >>> It is probably good to be more safe with an extra check of 
> >>> evsel__is_bpf(). But it is not required with current code. 
> >> 
> >> hum, but it will do all the affinity setup no? for no reason,
> >> if there's no non-bpb event
> > 
> > Yes, it will do the affinity setup. Let me see how to get something
> > like all_counters_use_bpf here (or within builtin-stat.c).
> > 
> 
> Would something like the following work? It is not clean (skipping some 
> useful logic in __evlist__[enable|disable]). But it seems to work in the
> tests.

sorry for late reply, but I can't no longer apply this:

	patching file tools/perf/builtin-stat.c
	Hunk #1 FAILED at 572.
	Hunk #2 FAILED at 581.
	2 out of 2 hunks FAILED -- saving rejects to file tools/perf/builtin-stat.c.rej
	patching file tools/perf/util/evlist.c
	Hunk #1 FAILED at 425.
	1 out of 1 hunk FAILED -- saving rejects to file tools/perf/util/evlist.c.rej

ah, I see the patchset got already merged.. not sure why I'm doing review then ;-)

thanks,
jirka

Re: [PATCH v5 5/5] perf-stat: introduce bpf_counter_ops->disable()

[Arnaldo Carvalho de Melo]

Em Mon, May 03, 2021 at 04:09:45PM +0200, Jiri Olsa escreveu:
> On Thu, Apr 29, 2021 at 10:40:01PM +0000, Song Liu wrote:
> 
> SNIP
> 
> > >>>>> #include "../perf.h"
> > >>>>> @@ -421,6 +422,9 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
> > >>>>> 	if (affinity__setup(&affinity) < 0)
> > >>>>> 		return;

> > >>>>> +	evlist__for_each_entry(evlist, pos)
> > >>>>> +		bpf_counter__disable(pos);

> > >>>> I was wondering why you don't check evsel__is_bpf like
> > >>>> for the enable case.. and realized that we don't skip
> > >>>> bpf evsels in __evlist__enable and __evlist__disable
> > >>>> like we do in read_affinity_counters

> > >>>> so I guess there's extra affinity setup and bunch of
> > >>>> wrong ioctls being called?

> > >>> We actually didn't do wrong ioctls because the following check:

> > >>>      if (... || !pos->core.fd)
> > >>>               continue;

> > >>> in __evlist__enable and __evlist__disable. That we don't allocate 
> > >>> core.fd for is_bpf events. 

> > >>> It is probably good to be more safe with an extra check of 
> > >>> evsel__is_bpf(). But it is not required with current code. 

> > >> hum, but it will do all the affinity setup no? for no reason,
> > >> if there's no non-bpb event

> > > Yes, it will do the affinity setup. Let me see how to get something
> > > like all_counters_use_bpf here (or within builtin-stat.c).

> > Would something like the following work? It is not clean (skipping some 
> > useful logic in __evlist__[enable|disable]). But it seems to work in the
> > tests.

> sorry for late reply, but I can't no longer apply this:
 
> 	patching file tools/perf/builtin-stat.c
> 	Hunk #1 FAILED at 572.
> 	Hunk #2 FAILED at 581.
> 	2 out of 2 hunks FAILED -- saving rejects to file tools/perf/builtin-stat.c.rej
> 	patching file tools/perf/util/evlist.c
> 	Hunk #1 FAILED at 425.
> 	1 out of 1 hunk FAILED -- saving rejects to file tools/perf/util/evlist.c.rej
 
> ah, I see the patchset got already merged.. not sure why I'm doing review then ;-)

Hey, sometimes this can happen, sorry. Song, please submit on top of
what is upstream.

- Arnaldo