[PATCHv2 0/3] perf/bpf: Replace deprecated code

[PATCHv2 0/3] perf/bpf: Replace deprecated code

[Jiri Olsa]

hi,
the original patchset [1] removed the whole perf functionality
with the hope nobody's using that. But it turned out there's
actually bpf script using prologue functionality, so there
might be users of this.

This patchset gets rid of and adds workaround (and keeps the
current functionality) for following deprecated libbpf
functions/struct:

  bpf_program__set_priv
  bpf_program__priv
  bpf_map__set_priv
  bpf_map__priv
  bpf_program__set_prep
  bpf_program__nth_fd
  struct bpf_prog_prep_result

Basically it implements workarounds suggested by Andrii in [2].

I tested with script from examples/bpf that are working for me:

  examples/bpf/hello.c
  examples/bpf/5sec.c

The rest seem to fail for various reasons even without this
change..  they seem unmaintained for some time now, but I might
have wrong setup.

Also available in here:
  git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  perf/depre

thanks,
jirka


[1] https://lore.kernel.org/linux-perf-users/YgoPxhE3OEEmZqla@krava/T/#t
[2] https://lore.kernel.org/linux-perf-users/YgoPxhE3OEEmZqla@krava/T/#md3ccab9fe70a4583e94603b1a562e369bd67b17d
---
Jiri Olsa (3):
      perf tools: Remove bpf_program__set_priv/bpf_program__priv usage
      perf tools: Remove bpf_map__set_priv/bpf_map__priv usage
      perf tools: Rework prologue generation code

 tools/perf/util/bpf-loader.c | 267 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 230 insertions(+), 37 deletions(-)

[PATCH 1/3] perf tools: Remove bpf_program__set_priv/bpf_program__priv usage

[Jiri Olsa]

Both bpf_program__set_priv/bpf_program__priv are deprecated
and will be eventually removed.

Using hashmap to replace that functionality.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/bpf-loader.c | 91 +++++++++++++++++++++++++++++-------
 1 file changed, 75 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index db61e09be585..ec27aab2bd36 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -26,6 +26,7 @@
 #include "util.h"
 #include "llvm-utils.h"
 #include "c++/clang-c.h"
+#include "hashmap.h"
 
 #include <internal/xyarray.h>
 
@@ -55,6 +56,7 @@ struct bpf_perf_object {
 };
 
 static LIST_HEAD(bpf_objects_list);
+static struct hashmap *bpf_program_hash;
 
 static struct bpf_perf_object *
 bpf_perf_object__next(struct bpf_perf_object *prev)
@@ -173,6 +175,35 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
 	return obj;
 }
 
+static void
+clear_prog_priv(const struct bpf_program *prog __maybe_unused,
+		void *_priv)
+{
+	struct bpf_prog_priv *priv = _priv;
+
+	cleanup_perf_probe_events(&priv->pev, 1);
+	zfree(&priv->insns_buf);
+	zfree(&priv->type_mapping);
+	zfree(&priv->sys_name);
+	zfree(&priv->evt_name);
+	free(priv);
+}
+
+static void bpf_program_hash_free(void)
+{
+	struct hashmap_entry *cur;
+	size_t bkt;
+
+	if (!bpf_program_hash)
+		return;
+
+	hashmap__for_each_entry(bpf_program_hash, cur, bkt)
+		clear_prog_priv(cur->key, cur->value);
+
+	hashmap__free(bpf_program_hash);
+	bpf_program_hash = NULL;
+}
+
 void bpf__clear(void)
 {
 	struct bpf_perf_object *perf_obj, *tmp;
@@ -181,20 +212,48 @@ void bpf__clear(void)
 		bpf__unprobe(perf_obj->obj);
 		bpf_perf_object__close(perf_obj);
 	}
+
+	bpf_program_hash_free();
 }
 
-static void
-clear_prog_priv(struct bpf_program *prog __maybe_unused,
-		void *_priv)
+static size_t ptr_hash(const void *__key, void *ctx __maybe_unused)
 {
-	struct bpf_prog_priv *priv = _priv;
+	return (size_t) __key;
+}
 
-	cleanup_perf_probe_events(&priv->pev, 1);
-	zfree(&priv->insns_buf);
-	zfree(&priv->type_mapping);
-	zfree(&priv->sys_name);
-	zfree(&priv->evt_name);
-	free(priv);
+static bool ptr_equal(const void *key1, const void *key2,
+			  void *ctx __maybe_unused)
+{
+	return key1 == key2;
+}
+
+static void *program_priv(const struct bpf_program *prog)
+{
+	void *priv;
+
+	if (!bpf_program_hash)
+		return NULL;
+	if (!hashmap__find(bpf_program_hash, prog, &priv))
+		return NULL;
+	return priv;
+}
+
+static int program_set_priv(struct bpf_program *prog, void *priv)
+{
+	void *old_priv;
+
+	if (!bpf_program_hash) {
+		bpf_program_hash = hashmap__new(ptr_hash, ptr_equal, NULL);
+		if (!bpf_program_hash)
+			return -ENOMEM;
+	}
+
+	old_priv = program_priv(prog);
+	if (old_priv) {
+		clear_prog_priv(prog, old_priv);
+		return hashmap__set(bpf_program_hash, prog, priv, NULL, NULL);
+	}
+	return hashmap__add(bpf_program_hash, prog, priv);
 }
 
 static int
@@ -438,7 +497,7 @@ config_bpf_program(struct bpf_program *prog)
 	pr_debug("bpf: config '%s' is ok\n", config_str);
 
 set_priv:
-	err = bpf_program__set_priv(prog, priv, clear_prog_priv);
+	err = program_set_priv(prog, priv);
 	if (err) {
 		pr_debug("Failed to set priv for program '%s'\n", config_str);
 		goto errout;
@@ -479,7 +538,7 @@ preproc_gen_prologue(struct bpf_program *prog, int n,
 		     struct bpf_insn *orig_insns, int orig_insns_cnt,
 		     struct bpf_prog_prep_result *res)
 {
-	struct bpf_prog_priv *priv = bpf_program__priv(prog);
+	struct bpf_prog_priv *priv = program_priv(prog);
 	struct probe_trace_event *tev;
 	struct perf_probe_event *pev;
 	struct bpf_insn *buf;
@@ -630,7 +689,7 @@ static int map_prologue(struct perf_probe_event *pev, int *mapping,
 
 static int hook_load_preprocessor(struct bpf_program *prog)
 {
-	struct bpf_prog_priv *priv = bpf_program__priv(prog);
+	struct bpf_prog_priv *priv = program_priv(prog);
 	struct perf_probe_event *pev;
 	bool need_prologue = false;
 	int err, i;
@@ -706,7 +765,7 @@ int bpf__probe(struct bpf_object *obj)
 		if (err)
 			goto out;
 
-		priv = bpf_program__priv(prog);
+		priv = program_priv(prog);
 		if (IS_ERR_OR_NULL(priv)) {
 			if (!priv)
 				err = -BPF_LOADER_ERRNO__INTERNAL;
@@ -758,7 +817,7 @@ int bpf__unprobe(struct bpf_object *obj)
 	struct bpf_program *prog;
 
 	bpf_object__for_each_program(prog, obj) {
-		struct bpf_prog_priv *priv = bpf_program__priv(prog);
+		struct bpf_prog_priv *priv = program_priv(prog);
 		int i;
 
 		if (IS_ERR_OR_NULL(priv) || priv->is_tp)
@@ -814,7 +873,7 @@ int bpf__foreach_event(struct bpf_object *obj,
 	int err;
 
 	bpf_object__for_each_program(prog, obj) {
-		struct bpf_prog_priv *priv = bpf_program__priv(prog);
+		struct bpf_prog_priv *priv = program_priv(prog);
 		struct probe_trace_event *tev;
 		struct perf_probe_event *pev;
 		int i, fd;
-- 
2.35.1

[PATCH 2/3] perf tools: Remove bpf_map__set_priv/bpf_map__priv usage

[Jiri Olsa]

Both bpf_map__set_priv/bpf_map__priv are deprecated
and will be eventually removed.

Using hashmap to replace that functionality.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/bpf-loader.c | 62 ++++++++++++++++++++++++++++++++----
 1 file changed, 55 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index ec27aab2bd36..6f87729817ad 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -57,6 +57,7 @@ struct bpf_perf_object {
 
 static LIST_HEAD(bpf_objects_list);
 static struct hashmap *bpf_program_hash;
+static struct hashmap *bpf_map_hash;
 
 static struct bpf_perf_object *
 bpf_perf_object__next(struct bpf_perf_object *prev)
@@ -204,6 +205,8 @@ static void bpf_program_hash_free(void)
 	bpf_program_hash = NULL;
 }
 
+static void bpf_map_hash_free(void);
+
 void bpf__clear(void)
 {
 	struct bpf_perf_object *perf_obj, *tmp;
@@ -214,6 +217,7 @@ void bpf__clear(void)
 	}
 
 	bpf_program_hash_free();
+	bpf_map_hash_free();
 }
 
 static size_t ptr_hash(const void *__key, void *ctx __maybe_unused)
@@ -969,7 +973,7 @@ bpf_map_priv__purge(struct bpf_map_priv *priv)
 }
 
 static void
-bpf_map_priv__clear(struct bpf_map *map __maybe_unused,
+bpf_map_priv__clear(const struct bpf_map *map __maybe_unused,
 		    void *_priv)
 {
 	struct bpf_map_priv *priv = _priv;
@@ -978,6 +982,50 @@ bpf_map_priv__clear(struct bpf_map *map __maybe_unused,
 	free(priv);
 }
 
+static void *map_priv(const struct bpf_map *map)
+{
+	void *priv;
+
+	if (!bpf_map_hash)
+		return NULL;
+	if (!hashmap__find(bpf_map_hash, map, &priv))
+		return NULL;
+	return priv;
+}
+
+static void bpf_map_hash_free(void)
+{
+	struct hashmap_entry *cur;
+	size_t bkt;
+
+	if (!bpf_map_hash)
+		return;
+
+	hashmap__for_each_entry(bpf_map_hash, cur, bkt)
+		bpf_map_priv__clear(cur->key, cur->value);
+
+	hashmap__free(bpf_map_hash);
+	bpf_map_hash = NULL;
+}
+
+static int map_set_priv(struct bpf_map *map, void *priv)
+{
+	void *old_priv;
+
+	if (!bpf_map_hash) {
+		bpf_map_hash = hashmap__new(ptr_hash, ptr_equal, NULL);
+		if (!bpf_map_hash)
+			return -ENOMEM;
+	}
+
+	old_priv = map_priv(map);
+	if (old_priv) {
+		bpf_map_priv__clear(map, old_priv);
+		return hashmap__set(bpf_map_hash, map, priv, NULL, NULL);
+	}
+	return hashmap__add(bpf_map_hash, map, priv);
+}
+
 static int
 bpf_map_op_setkey(struct bpf_map_op *op, struct parse_events_term *term)
 {
@@ -1077,7 +1125,7 @@ static int
 bpf_map__add_op(struct bpf_map *map, struct bpf_map_op *op)
 {
 	const char *map_name = bpf_map__name(map);
-	struct bpf_map_priv *priv = bpf_map__priv(map);
+	struct bpf_map_priv *priv = map_priv(map);
 
 	if (IS_ERR(priv)) {
 		pr_debug("Failed to get private from map %s\n", map_name);
@@ -1092,7 +1140,7 @@ bpf_map__add_op(struct bpf_map *map, struct bpf_map_op *op)
 		}
 		INIT_LIST_HEAD(&priv->ops_list);
 
-		if (bpf_map__set_priv(map, priv, bpf_map_priv__clear)) {
+		if (map_set_priv(map, priv)) {
 			free(priv);
 			return -BPF_LOADER_ERRNO__INTERNAL;
 		}
@@ -1432,7 +1480,7 @@ bpf_map_config_foreach_key(struct bpf_map *map,
 	struct bpf_map_op *op;
 	const struct bpf_map_def *def;
 	const char *name = bpf_map__name(map);
-	struct bpf_map_priv *priv = bpf_map__priv(map);
+	struct bpf_map_priv *priv = map_priv(map);
 
 	if (IS_ERR(priv)) {
 		pr_debug("ERROR: failed to get private from map %s\n", name);
@@ -1652,7 +1700,7 @@ struct evsel *bpf__setup_output_event(struct evlist *evlist, const char *name)
 	bool need_init = false;
 
 	bpf__perf_for_each_map_named(map, perf_obj, tmp, name) {
-		struct bpf_map_priv *priv = bpf_map__priv(map);
+		struct bpf_map_priv *priv = map_priv(map);
 
 		if (IS_ERR(priv))
 			return ERR_PTR(-BPF_LOADER_ERRNO__INTERNAL);
@@ -1688,7 +1736,7 @@ struct evsel *bpf__setup_output_event(struct evlist *evlist, const char *name)
 	}
 
 	bpf__perf_for_each_map_named(map, perf_obj, tmp, name) {
-		struct bpf_map_priv *priv = bpf_map__priv(map);
+		struct bpf_map_priv *priv = map_priv(map);
 
 		if (IS_ERR(priv))
 			return ERR_PTR(-BPF_LOADER_ERRNO__INTERNAL);
@@ -1700,7 +1748,7 @@ struct evsel *bpf__setup_output_event(struct evlist *evlist, const char *name)
 			if (!priv)
 				return ERR_PTR(-ENOMEM);
 
-			err = bpf_map__set_priv(map, priv, bpf_map_priv__clear);
+			err = map_set_priv(map, priv);
 			if (err) {
 				bpf_map_priv__clear(map, priv);
 				return ERR_PTR(err);
-- 
2.35.1

[PATCH 3/3] perf tools: Rework prologue generation code

[Jiri Olsa]

Some functions we use now for bpf prologue generation are
going to be deprecated, so reworking the current code not
to use them.

We need to replace following functions/struct:
   bpf_program__set_prep
   bpf_program__nth_fd
   struct bpf_prog_prep_result

Current code uses bpf_program__set_prep to hook perf callback
before the program is loaded and provide new instructions with
the prologue.

We workaround this by using objects's 'unloaded' programs instructions
for that specific program and load new ebpf programs with prologue
using separate bpf_prog_load calls.

We keep new ebpf program instances descriptors in bpf programs
private struct.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
 1 file changed, 104 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index 6f87729817ad..03f917002c00 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -48,6 +48,7 @@ struct bpf_prog_priv {
 	struct bpf_insn *insns_buf;
 	int nr_types;
 	int *type_mapping;
+	int *proglogue_fds;
 };
 
 struct bpf_perf_object {
@@ -55,6 +56,11 @@ struct bpf_perf_object {
 	struct bpf_object *obj;
 };
 
+struct bpf_preproc_result {
+	struct bpf_insn *new_insn_ptr;
+	int new_insn_cnt;
+};
+
 static LIST_HEAD(bpf_objects_list);
 static struct hashmap *bpf_program_hash;
 static struct hashmap *bpf_map_hash;
@@ -176,14 +182,31 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
 	return obj;
 }
 
+static void close_prologue_programs(struct bpf_prog_priv *priv)
+{
+	struct perf_probe_event *pev;
+	int i, fd;
+
+	if (!priv->need_prologue)
+		return;
+	pev = &priv->pev;
+	for (i = 0; i < pev->ntevs; i++) {
+		fd = close(priv->proglogue_fds[i]);
+		if (fd != -1)
+			close(fd);
+	}
+}
+
 static void
 clear_prog_priv(const struct bpf_program *prog __maybe_unused,
 		void *_priv)
 {
 	struct bpf_prog_priv *priv = _priv;
 
+	close_prologue_programs(priv);
 	cleanup_perf_probe_events(&priv->pev, 1);
 	zfree(&priv->insns_buf);
+	zfree(&priv->proglogue_fds);
 	zfree(&priv->type_mapping);
 	zfree(&priv->sys_name);
 	zfree(&priv->evt_name);
@@ -539,8 +562,8 @@ static int bpf__prepare_probe(void)
 
 static int
 preproc_gen_prologue(struct bpf_program *prog, int n,
-		     struct bpf_insn *orig_insns, int orig_insns_cnt,
-		     struct bpf_prog_prep_result *res)
+		     const struct bpf_insn *orig_insns, int orig_insns_cnt,
+		     struct bpf_preproc_result *res)
 {
 	struct bpf_prog_priv *priv = program_priv(prog);
 	struct probe_trace_event *tev;
@@ -588,7 +611,6 @@ preproc_gen_prologue(struct bpf_program *prog, int n,
 
 	res->new_insn_ptr = buf;
 	res->new_insn_cnt = prologue_cnt + orig_insns_cnt;
-	res->pfd = NULL;
 	return 0;
 
 errout:
@@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
 	struct bpf_prog_priv *priv = program_priv(prog);
 	struct perf_probe_event *pev;
 	bool need_prologue = false;
-	int err, i;
+	int i;
 
 	if (IS_ERR_OR_NULL(priv)) {
 		pr_debug("Internal error when hook preprocessor\n");
@@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
 		return 0;
 	}
 
+	/*
+	 * Do not load programs that need prologue, because we need
+	 * to add prologue first, check bpf_object__load_prologue.
+	 */
+	bpf_program__set_autoload(prog, false);
+
 	priv->need_prologue = true;
 	priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
 	if (!priv->insns_buf) {
@@ -734,6 +762,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
 		return -ENOMEM;
 	}
 
+	priv->proglogue_fds = malloc(sizeof(int) * pev->ntevs);
+	if (!priv->proglogue_fds) {
+		pr_debug("Not enough memory: alloc prologue fds failed\n");
+		return -ENOMEM;
+	}
+	memset(priv->proglogue_fds, -1, sizeof(int) * pev->ntevs);
+
 	priv->type_mapping = malloc(sizeof(int) * pev->ntevs);
 	if (!priv->type_mapping) {
 		pr_debug("Not enough memory: alloc type_mapping failed\n");
@@ -742,13 +777,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
 	memset(priv->type_mapping, -1,
 	       sizeof(int) * pev->ntevs);
 
-	err = map_prologue(pev, priv->type_mapping, &priv->nr_types);
-	if (err)
-		return err;
-
-	err = bpf_program__set_prep(prog, priv->nr_types,
-				    preproc_gen_prologue);
-	return err;
+	return map_prologue(pev, priv->type_mapping, &priv->nr_types);
 }
 
 int bpf__probe(struct bpf_object *obj)
@@ -855,6 +884,66 @@ int bpf__unprobe(struct bpf_object *obj)
 	return ret;
 }
 
+static int bpf_object__load_prologue(struct bpf_object *obj)
+{
+	const struct bpf_insn *orig_insns;
+	struct bpf_preproc_result res;
+	struct perf_probe_event *pev;
+	struct bpf_program *prog;
+	int orig_insns_cnt;
+
+	bpf_object__for_each_program(prog, obj) {
+		struct bpf_prog_priv *priv = program_priv(prog);
+		int err, i, fd;
+
+		if (IS_ERR_OR_NULL(priv)) {
+			pr_debug("bpf: failed to get private field\n");
+			return -BPF_LOADER_ERRNO__INTERNAL;
+		}
+
+		if (!priv->need_prologue)
+			continue;
+
+		/*
+		 * For each program that needs prologue we do following:
+		 *
+		 * - take its current instructions and use them
+		 *   to generate the new code with prologue
+		 *
+		 * - load new instructions with bpf_prog_load
+		 *   and keep the fd in proglogue_fds
+		 *
+		 * - new fd will be used bpf__foreach_event
+		 *   to connect this program with perf evsel
+		 */
+		orig_insns = bpf_program__insns(prog);
+		orig_insns_cnt = bpf_program__insn_cnt(prog);
+
+		pev = &priv->pev;
+		for (i = 0; i < pev->ntevs; i++) {
+			err = preproc_gen_prologue(prog, i, orig_insns,
+						   orig_insns_cnt, &res);
+			if (err)
+				return err;
+
+			fd = bpf_prog_load(bpf_program__get_type(prog),
+					   bpf_program__name(prog), "GPL",
+					   res.new_insn_ptr,
+					   res.new_insn_cnt, NULL);
+			if (fd < 0) {
+				char bf[128];
+
+				libbpf_strerror(-errno, bf, sizeof(bf));
+				pr_debug("bpf: load objects with prologue failed: err=%d: (%s)\n",
+					 -errno, bf);
+				return -errno;
+			}
+			priv->proglogue_fds[i] = fd;
+		}
+	}
+	return 0;
+}
+
 int bpf__load(struct bpf_object *obj)
 {
 	int err;
@@ -866,7 +955,7 @@ int bpf__load(struct bpf_object *obj)
 		pr_debug("bpf: load objects failed: err=%d: (%s)\n", err, bf);
 		return err;
 	}
-	return 0;
+	return bpf_object__load_prologue(obj);
 }
 
 int bpf__foreach_event(struct bpf_object *obj,
@@ -901,13 +990,10 @@ int bpf__foreach_event(struct bpf_object *obj,
 		for (i = 0; i < pev->ntevs; i++) {
 			tev = &pev->tevs[i];
 
-			if (priv->need_prologue) {
-				int type = priv->type_mapping[i];
-
-				fd = bpf_program__nth_fd(prog, type);
-			} else {
+			if (priv->need_prologue)
+				fd = priv->proglogue_fds[i];
+			else
 				fd = bpf_program__fd(prog);
-			}
 
 			if (fd < 0) {
 				pr_debug("bpf: failed to get file descriptor\n");
-- 
2.35.1

Re: [PATCH 1/3] perf tools: Remove bpf_program__set_priv/bpf_program__priv usage

[Andrii Nakryiko]

On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Both bpf_program__set_priv/bpf_program__priv are deprecated
> and will be eventually removed.
>
> Using hashmap to replace that functionality.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/bpf-loader.c | 91 +++++++++++++++++++++++++++++-------
>  1 file changed, 75 insertions(+), 16 deletions(-)
>

[...]

> +
> +static int program_set_priv(struct bpf_program *prog, void *priv)
> +{
> +       void *old_priv;
> +
> +       if (!bpf_program_hash) {
> +               bpf_program_hash = hashmap__new(ptr_hash, ptr_equal, NULL);
> +               if (!bpf_program_hash)

should use IS_ERR here

> +                       return -ENOMEM;
> +       }
> +
> +       old_priv = program_priv(prog);
> +       if (old_priv) {
> +               clear_prog_priv(prog, old_priv);
> +               return hashmap__set(bpf_program_hash, prog, priv, NULL, NULL);
> +       }
> +       return hashmap__add(bpf_program_hash, prog, priv);
>  }

[...]

Re: [PATCH 2/3] perf tools: Remove bpf_map__set_priv/bpf_map__priv usage

[Andrii Nakryiko]

On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Both bpf_map__set_priv/bpf_map__priv are deprecated
> and will be eventually removed.
>
> Using hashmap to replace that functionality.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/bpf-loader.c | 62 ++++++++++++++++++++++++++++++++----
>  1 file changed, 55 insertions(+), 7 deletions(-)
>

[...]

> +static int map_set_priv(struct bpf_map *map, void *priv)
> +{
> +       void *old_priv;
> +
> +       if (!bpf_map_hash) {
> +               bpf_map_hash = hashmap__new(ptr_hash, ptr_equal, NULL);
> +               if (!bpf_map_hash)

same as in previous patch, on error this is not going to be NULL

> +                       return -ENOMEM;
> +       }
> +
> +       old_priv = map_priv(map);
> +       if (old_priv) {
> +               bpf_map_priv__clear(map, old_priv);
> +               return hashmap__set(bpf_map_hash, map, priv, NULL, NULL);
> +       }
> +       return hashmap__add(bpf_map_hash, map, priv);
> +}
> +

[...]

Re: [PATCH 3/3] perf tools: Rework prologue generation code

[Andrii Nakryiko]

On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Some functions we use now for bpf prologue generation are
> going to be deprecated, so reworking the current code not
> to use them.
>
> We need to replace following functions/struct:
>    bpf_program__set_prep
>    bpf_program__nth_fd
>    struct bpf_prog_prep_result
>
> Current code uses bpf_program__set_prep to hook perf callback
> before the program is loaded and provide new instructions with
> the prologue.
>
> We workaround this by using objects's 'unloaded' programs instructions
> for that specific program and load new ebpf programs with prologue
> using separate bpf_prog_load calls.
>
> We keep new ebpf program instances descriptors in bpf programs
> private struct.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
>  1 file changed, 104 insertions(+), 18 deletions(-)
>

[...]

>  errout:
> @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
>         struct bpf_prog_priv *priv = program_priv(prog);
>         struct perf_probe_event *pev;
>         bool need_prologue = false;
> -       int err, i;
> +       int i;
>
>         if (IS_ERR_OR_NULL(priv)) {
>                 pr_debug("Internal error when hook preprocessor\n");
> @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
>                 return 0;
>         }
>
> +       /*
> +        * Do not load programs that need prologue, because we need
> +        * to add prologue first, check bpf_object__load_prologue.
> +        */
> +       bpf_program__set_autoload(prog, false);

if you set autoload to false, program instructions might be invalid in
the end. Libbpf doesn't apply some (all?) relocations to such
programs, doesn't resolve CO-RE, etc, etc. You have to let
"prototypal" BPF program to be loaded before you can grab final
instructions. It's not great, but in your case it should work, right?

> +
>         priv->need_prologue = true;
>         priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
>         if (!priv->insns_buf) {
> @@ -734,6 +762,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
>                 return -ENOMEM;
>         }
>

[...]

> +               /*
> +                * For each program that needs prologue we do following:
> +                *
> +                * - take its current instructions and use them
> +                *   to generate the new code with prologue
> +                *
> +                * - load new instructions with bpf_prog_load
> +                *   and keep the fd in proglogue_fds
> +                *
> +                * - new fd will be used bpf__foreach_event
> +                *   to connect this program with perf evsel
> +                */
> +               orig_insns = bpf_program__insns(prog);
> +               orig_insns_cnt = bpf_program__insn_cnt(prog);
> +
> +               pev = &priv->pev;
> +               for (i = 0; i < pev->ntevs; i++) {
> +                       err = preproc_gen_prologue(prog, i, orig_insns,
> +                                                  orig_insns_cnt, &res);
> +                       if (err)
> +                               return err;
> +
> +                       fd = bpf_prog_load(bpf_program__get_type(prog),

nit: bpf_program__type() is preferred (we are deprecating/discouraging
"get_" prefixed getters in libbpf 1.0)

> +                                          bpf_program__name(prog), "GPL",

would it make sense to give each clone a distinct name?

> +                                          res.new_insn_ptr,
> +                                          res.new_insn_cnt, NULL);
> +                       if (fd < 0) {
> +                               char bf[128];
> +

[...]

Re: [PATCHv2 0/3] perf/bpf: Replace deprecated code

[Andrii Nakryiko]

On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> hi,
> the original patchset [1] removed the whole perf functionality
> with the hope nobody's using that. But it turned out there's
> actually bpf script using prologue functionality, so there
> might be users of this.
>
> This patchset gets rid of and adds workaround (and keeps the
> current functionality) for following deprecated libbpf
> functions/struct:
>
>   bpf_program__set_priv
>   bpf_program__priv
>   bpf_map__set_priv
>   bpf_map__priv
>   bpf_program__set_prep
>   bpf_program__nth_fd
>   struct bpf_prog_prep_result
>
> Basically it implements workarounds suggested by Andrii in [2].
>
> I tested with script from examples/bpf that are working for me:
>
>   examples/bpf/hello.c
>   examples/bpf/5sec.c
>
> The rest seem to fail for various reasons even without this
> change..  they seem unmaintained for some time now, but I might
> have wrong setup.
>
> Also available in here:
>   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   perf/depre
>
> thanks,
> jirka
>
>
> [1] https://lore.kernel.org/linux-perf-users/YgoPxhE3OEEmZqla@krava/T/#t
> [2] https://lore.kernel.org/linux-perf-users/YgoPxhE3OEEmZqla@krava/T/#md3ccab9fe70a4583e94603b1a562e369bd67b17d
> ---
> Jiri Olsa (3):
>       perf tools: Remove bpf_program__set_priv/bpf_program__priv usage
>       perf tools: Remove bpf_map__set_priv/bpf_map__priv usage
>       perf tools: Rework prologue generation code
>

It's great that you are deprecating these, thanks a lot for that! I
suggest to also doing libbpf_set_strict_mode(LIBBPF_STRICT_ALL) to
check that libbpf 1.0 won't break anything. For example, you'll need
to use a custom SEC() handler to handle those quirky sections that
perf allows. This patch set has landed in bpf-next, so you should be
good to go.


>  tools/perf/util/bpf-loader.c | 267 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 230 insertions(+), 37 deletions(-)

Re: [PATCH 1/3] perf tools: Remove bpf_program__set_priv/bpf_program__priv usage

[Jiri Olsa]

On Thu, Feb 17, 2022 at 01:47:16PM -0800, Andrii Nakryiko wrote:
> On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Both bpf_program__set_priv/bpf_program__priv are deprecated
> > and will be eventually removed.
> >
> > Using hashmap to replace that functionality.
> >
> > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/util/bpf-loader.c | 91 +++++++++++++++++++++++++++++-------
> >  1 file changed, 75 insertions(+), 16 deletions(-)
> >
> 
> [...]
> 
> > +
> > +static int program_set_priv(struct bpf_program *prog, void *priv)
> > +{
> > +       void *old_priv;
> > +
> > +       if (!bpf_program_hash) {
> > +               bpf_program_hash = hashmap__new(ptr_hash, ptr_equal, NULL);
> > +               if (!bpf_program_hash)
> 
> should use IS_ERR here

ah right, thanks

jirka

> 
> > +                       return -ENOMEM;
> > +       }
> > +
> > +       old_priv = program_priv(prog);
> > +       if (old_priv) {
> > +               clear_prog_priv(prog, old_priv);
> > +               return hashmap__set(bpf_program_hash, prog, priv, NULL, NULL);
> > +       }
> > +       return hashmap__add(bpf_program_hash, prog, priv);
> >  }
> 
> [...]

Re: [PATCH 2/3] perf tools: Remove bpf_map__set_priv/bpf_map__priv usage

[Jiri Olsa]

On Thu, Feb 17, 2022 at 01:49:39PM -0800, Andrii Nakryiko wrote:
> On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Both bpf_map__set_priv/bpf_map__priv are deprecated
> > and will be eventually removed.
> >
> > Using hashmap to replace that functionality.
> >
> > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/util/bpf-loader.c | 62 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 55 insertions(+), 7 deletions(-)
> >
> 
> [...]
> 
> > +static int map_set_priv(struct bpf_map *map, void *priv)
> > +{
> > +       void *old_priv;
> > +
> > +       if (!bpf_map_hash) {
> > +               bpf_map_hash = hashmap__new(ptr_hash, ptr_equal, NULL);
> > +               if (!bpf_map_hash)
> 
> same as in previous patch, on error this is not going to be NULL

yes, will change

jirka

> 
> > +                       return -ENOMEM;
> > +       }
> > +
> > +       old_priv = map_priv(map);
> > +       if (old_priv) {
> > +               bpf_map_priv__clear(map, old_priv);
> > +               return hashmap__set(bpf_map_hash, map, priv, NULL, NULL);
> > +       }
> > +       return hashmap__add(bpf_map_hash, map, priv);
> > +}
> > +
> 
> [...]

Re: [PATCH 3/3] perf tools: Rework prologue generation code

[Jiri Olsa]

On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Some functions we use now for bpf prologue generation are
> > going to be deprecated, so reworking the current code not
> > to use them.
> >
> > We need to replace following functions/struct:
> >    bpf_program__set_prep
> >    bpf_program__nth_fd
> >    struct bpf_prog_prep_result
> >
> > Current code uses bpf_program__set_prep to hook perf callback
> > before the program is loaded and provide new instructions with
> > the prologue.
> >
> > We workaround this by using objects's 'unloaded' programs instructions
> > for that specific program and load new ebpf programs with prologue
> > using separate bpf_prog_load calls.
> >
> > We keep new ebpf program instances descriptors in bpf programs
> > private struct.
> >
> > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> >  1 file changed, 104 insertions(+), 18 deletions(-)
> >
> 
> [...]
> 
> >  errout:
> > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> >         struct bpf_prog_priv *priv = program_priv(prog);
> >         struct perf_probe_event *pev;
> >         bool need_prologue = false;
> > -       int err, i;
> > +       int i;
> >
> >         if (IS_ERR_OR_NULL(priv)) {
> >                 pr_debug("Internal error when hook preprocessor\n");
> > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> >                 return 0;
> >         }
> >
> > +       /*
> > +        * Do not load programs that need prologue, because we need
> > +        * to add prologue first, check bpf_object__load_prologue.
> > +        */
> > +       bpf_program__set_autoload(prog, false);
> 
> if you set autoload to false, program instructions might be invalid in
> the end. Libbpf doesn't apply some (all?) relocations to such
> programs, doesn't resolve CO-RE, etc, etc. You have to let
> "prototypal" BPF program to be loaded before you can grab final
> instructions. It's not great, but in your case it should work, right?

hum, do we care? it should all be done when the 'new' program with
the prologue is loaded, right?

I switched it off because the verifier failed to load the program
without the prologue.. because in the originaal program there's no
code to grab the arguments that the rest of the code depends on,
so the verifier sees invalid access

> 
> > +
> >         priv->need_prologue = true;
> >         priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
> >         if (!priv->insns_buf) {
> > @@ -734,6 +762,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> >                 return -ENOMEM;
> >         }
> >
> 
> [...]
> 
> > +               /*
> > +                * For each program that needs prologue we do following:
> > +                *
> > +                * - take its current instructions and use them
> > +                *   to generate the new code with prologue
> > +                *
> > +                * - load new instructions with bpf_prog_load
> > +                *   and keep the fd in proglogue_fds
> > +                *
> > +                * - new fd will be used bpf__foreach_event
> > +                *   to connect this program with perf evsel
> > +                */
> > +               orig_insns = bpf_program__insns(prog);
> > +               orig_insns_cnt = bpf_program__insn_cnt(prog);
> > +
> > +               pev = &priv->pev;
> > +               for (i = 0; i < pev->ntevs; i++) {
> > +                       err = preproc_gen_prologue(prog, i, orig_insns,
> > +                                                  orig_insns_cnt, &res);
> > +                       if (err)
> > +                               return err;
> > +
> > +                       fd = bpf_prog_load(bpf_program__get_type(prog),
> 
> nit: bpf_program__type() is preferred (we are deprecating/discouraging
> "get_" prefixed getters in libbpf 1.0)

ok, will change

> 
> > +                                          bpf_program__name(prog), "GPL",
> 
> would it make sense to give each clone a distinct name?

AFAICS the original code uses same prog name for instances,
so I'd rather keep it that way

thanks,
jirka

> 
> > +                                          res.new_insn_ptr,
> > +                                          res.new_insn_cnt, NULL);
> > +                       if (fd < 0) {
> > +                               char bf[128];
> > +
> 
> [...]

Re: [PATCHv2 0/3] perf/bpf: Replace deprecated code

[Jiri Olsa]

On Thu, Feb 17, 2022 at 01:55:13PM -0800, Andrii Nakryiko wrote:
> On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > hi,
> > the original patchset [1] removed the whole perf functionality
> > with the hope nobody's using that. But it turned out there's
> > actually bpf script using prologue functionality, so there
> > might be users of this.
> >
> > This patchset gets rid of and adds workaround (and keeps the
> > current functionality) for following deprecated libbpf
> > functions/struct:
> >
> >   bpf_program__set_priv
> >   bpf_program__priv
> >   bpf_map__set_priv
> >   bpf_map__priv
> >   bpf_program__set_prep
> >   bpf_program__nth_fd
> >   struct bpf_prog_prep_result
> >
> > Basically it implements workarounds suggested by Andrii in [2].
> >
> > I tested with script from examples/bpf that are working for me:
> >
> >   examples/bpf/hello.c
> >   examples/bpf/5sec.c
> >
> > The rest seem to fail for various reasons even without this
> > change..  they seem unmaintained for some time now, but I might
> > have wrong setup.
> >
> > Also available in here:
> >   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> >   perf/depre
> >
> > thanks,
> > jirka
> >
> >
> > [1] https://lore.kernel.org/linux-perf-users/YgoPxhE3OEEmZqla@krava/T/#t
> > [2] https://lore.kernel.org/linux-perf-users/YgoPxhE3OEEmZqla@krava/T/#md3ccab9fe70a4583e94603b1a562e369bd67b17d
> > ---
> > Jiri Olsa (3):
> >       perf tools: Remove bpf_program__set_priv/bpf_program__priv usage
> >       perf tools: Remove bpf_map__set_priv/bpf_map__priv usage
> >       perf tools: Rework prologue generation code
> >
> 
> It's great that you are deprecating these, thanks a lot for that! I
> suggest to also doing libbpf_set_strict_mode(LIBBPF_STRICT_ALL) to

will check

> check that libbpf 1.0 won't break anything. For example, you'll need
> to use a custom SEC() handler to handle those quirky sections that
> perf allows. This patch set has landed in bpf-next, so you should be
> good to go.

ah ok it already got merged.. I'll add it in new version

thanks,
jirka

> 
> 
> >  tools/perf/util/bpf-loader.c | 267 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 230 insertions(+), 37 deletions(-)

Re: [PATCH 3/3] perf tools: Rework prologue generation code

[Jiri Olsa]

On Fri, Feb 18, 2022 at 10:01:45AM +0100, Jiri Olsa wrote:
> On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> > On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Some functions we use now for bpf prologue generation are
> > > going to be deprecated, so reworking the current code not
> > > to use them.
> > >
> > > We need to replace following functions/struct:
> > >    bpf_program__set_prep
> > >    bpf_program__nth_fd
> > >    struct bpf_prog_prep_result
> > >
> > > Current code uses bpf_program__set_prep to hook perf callback
> > > before the program is loaded and provide new instructions with
> > > the prologue.
> > >
> > > We workaround this by using objects's 'unloaded' programs instructions
> > > for that specific program and load new ebpf programs with prologue
> > > using separate bpf_prog_load calls.
> > >
> > > We keep new ebpf program instances descriptors in bpf programs
> > > private struct.
> > >
> > > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > >  1 file changed, 104 insertions(+), 18 deletions(-)
> > >
> > 
> > [...]
> > 
> > >  errout:
> > > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > >         struct bpf_prog_priv *priv = program_priv(prog);
> > >         struct perf_probe_event *pev;
> > >         bool need_prologue = false;
> > > -       int err, i;
> > > +       int i;
> > >
> > >         if (IS_ERR_OR_NULL(priv)) {
> > >                 pr_debug("Internal error when hook preprocessor\n");
> > > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > >                 return 0;
> > >         }
> > >
> > > +       /*
> > > +        * Do not load programs that need prologue, because we need
> > > +        * to add prologue first, check bpf_object__load_prologue.
> > > +        */
> > > +       bpf_program__set_autoload(prog, false);
> > 
> > if you set autoload to false, program instructions might be invalid in
> > the end. Libbpf doesn't apply some (all?) relocations to such
> > programs, doesn't resolve CO-RE, etc, etc. You have to let
> > "prototypal" BPF program to be loaded before you can grab final
> > instructions. It's not great, but in your case it should work, right?
> 
> hum, do we care? it should all be done when the 'new' program with
> the prologue is loaded, right?
> 
> I switched it off because the verifier failed to load the program
> without the prologue.. because in the originaal program there's no
> code to grab the arguments that the rest of the code depends on,
> so the verifier sees invalid access
> 
> > 
> > > +
> > >         priv->need_prologue = true;
> > >         priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
> > >         if (!priv->insns_buf) {
> > > @@ -734,6 +762,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > >                 return -ENOMEM;
> > >         }
> > >
> > 
> > [...]
> > 
> > > +               /*
> > > +                * For each program that needs prologue we do following:
> > > +                *
> > > +                * - take its current instructions and use them
> > > +                *   to generate the new code with prologue
> > > +                *
> > > +                * - load new instructions with bpf_prog_load
> > > +                *   and keep the fd in proglogue_fds
> > > +                *
> > > +                * - new fd will be used bpf__foreach_event
> > > +                *   to connect this program with perf evsel
> > > +                */
> > > +               orig_insns = bpf_program__insns(prog);
> > > +               orig_insns_cnt = bpf_program__insn_cnt(prog);
> > > +
> > > +               pev = &priv->pev;
> > > +               for (i = 0; i < pev->ntevs; i++) {
> > > +                       err = preproc_gen_prologue(prog, i, orig_insns,
> > > +                                                  orig_insns_cnt, &res);
> > > +                       if (err)
> > > +                               return err;
> > > +
> > > +                       fd = bpf_prog_load(bpf_program__get_type(prog),
> > 
> > nit: bpf_program__type() is preferred (we are deprecating/discouraging
> > "get_" prefixed getters in libbpf 1.0)
> 
> ok, will change

hum, I can't see bpf_program__type.. what do I miss?

jirka

Re: [PATCH 3/3] perf tools: Rework prologue generation code

[Jiri Olsa]

On Fri, Feb 18, 2022 at 02:03:28PM +0100, Jiri Olsa wrote:
> On Fri, Feb 18, 2022 at 10:01:45AM +0100, Jiri Olsa wrote:
> > On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> > > On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > Some functions we use now for bpf prologue generation are
> > > > going to be deprecated, so reworking the current code not
> > > > to use them.
> > > >
> > > > We need to replace following functions/struct:
> > > >    bpf_program__set_prep
> > > >    bpf_program__nth_fd
> > > >    struct bpf_prog_prep_result
> > > >
> > > > Current code uses bpf_program__set_prep to hook perf callback
> > > > before the program is loaded and provide new instructions with
> > > > the prologue.
> > > >
> > > > We workaround this by using objects's 'unloaded' programs instructions
> > > > for that specific program and load new ebpf programs with prologue
> > > > using separate bpf_prog_load calls.
> > > >
> > > > We keep new ebpf program instances descriptors in bpf programs
> > > > private struct.
> > > >
> > > > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > > >  1 file changed, 104 insertions(+), 18 deletions(-)
> > > >
> > > 
> > > [...]
> > > 
> > > >  errout:
> > > > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > >         struct bpf_prog_priv *priv = program_priv(prog);
> > > >         struct perf_probe_event *pev;
> > > >         bool need_prologue = false;
> > > > -       int err, i;
> > > > +       int i;
> > > >
> > > >         if (IS_ERR_OR_NULL(priv)) {
> > > >                 pr_debug("Internal error when hook preprocessor\n");
> > > > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > >                 return 0;
> > > >         }
> > > >
> > > > +       /*
> > > > +        * Do not load programs that need prologue, because we need
> > > > +        * to add prologue first, check bpf_object__load_prologue.
> > > > +        */
> > > > +       bpf_program__set_autoload(prog, false);
> > > 
> > > if you set autoload to false, program instructions might be invalid in
> > > the end. Libbpf doesn't apply some (all?) relocations to such
> > > programs, doesn't resolve CO-RE, etc, etc. You have to let
> > > "prototypal" BPF program to be loaded before you can grab final
> > > instructions. It's not great, but in your case it should work, right?
> > 
> > hum, do we care? it should all be done when the 'new' program with
> > the prologue is loaded, right?
> > 
> > I switched it off because the verifier failed to load the program
> > without the prologue.. because in the originaal program there's no
> > code to grab the arguments that the rest of the code depends on,
> > so the verifier sees invalid access
> > 
> > > 
> > > > +
> > > >         priv->need_prologue = true;
> > > >         priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
> > > >         if (!priv->insns_buf) {
> > > > @@ -734,6 +762,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > >                 return -ENOMEM;
> > > >         }
> > > >
> > > 
> > > [...]
> > > 
> > > > +               /*
> > > > +                * For each program that needs prologue we do following:
> > > > +                *
> > > > +                * - take its current instructions and use them
> > > > +                *   to generate the new code with prologue
> > > > +                *
> > > > +                * - load new instructions with bpf_prog_load
> > > > +                *   and keep the fd in proglogue_fds
> > > > +                *
> > > > +                * - new fd will be used bpf__foreach_event
> > > > +                *   to connect this program with perf evsel
> > > > +                */
> > > > +               orig_insns = bpf_program__insns(prog);
> > > > +               orig_insns_cnt = bpf_program__insn_cnt(prog);
> > > > +
> > > > +               pev = &priv->pev;
> > > > +               for (i = 0; i < pev->ntevs; i++) {
> > > > +                       err = preproc_gen_prologue(prog, i, orig_insns,
> > > > +                                                  orig_insns_cnt, &res);
> > > > +                       if (err)
> > > > +                               return err;
> > > > +
> > > > +                       fd = bpf_prog_load(bpf_program__get_type(prog),
> > > 
> > > nit: bpf_program__type() is preferred (we are deprecating/discouraging
> > > "get_" prefixed getters in libbpf 1.0)
> > 
> > ok, will change
> 
> hum, I can't see bpf_program__type.. what do I miss?

nah I was on top of perf/core.. I see it now ;-)

jirka

Re: [PATCH 3/3] perf tools: Rework prologue generation code

[Andrii Nakryiko]

On Fri, Feb 18, 2022 at 1:01 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> > On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Some functions we use now for bpf prologue generation are
> > > going to be deprecated, so reworking the current code not
> > > to use them.
> > >
> > > We need to replace following functions/struct:
> > >    bpf_program__set_prep
> > >    bpf_program__nth_fd
> > >    struct bpf_prog_prep_result
> > >
> > > Current code uses bpf_program__set_prep to hook perf callback
> > > before the program is loaded and provide new instructions with
> > > the prologue.
> > >
> > > We workaround this by using objects's 'unloaded' programs instructions
> > > for that specific program and load new ebpf programs with prologue
> > > using separate bpf_prog_load calls.
> > >
> > > We keep new ebpf program instances descriptors in bpf programs
> > > private struct.
> > >
> > > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > >  1 file changed, 104 insertions(+), 18 deletions(-)
> > >
> >
> > [...]
> >
> > >  errout:
> > > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > >         struct bpf_prog_priv *priv = program_priv(prog);
> > >         struct perf_probe_event *pev;
> > >         bool need_prologue = false;
> > > -       int err, i;
> > > +       int i;
> > >
> > >         if (IS_ERR_OR_NULL(priv)) {
> > >                 pr_debug("Internal error when hook preprocessor\n");
> > > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > >                 return 0;
> > >         }
> > >
> > > +       /*
> > > +        * Do not load programs that need prologue, because we need
> > > +        * to add prologue first, check bpf_object__load_prologue.
> > > +        */
> > > +       bpf_program__set_autoload(prog, false);
> >
> > if you set autoload to false, program instructions might be invalid in
> > the end. Libbpf doesn't apply some (all?) relocations to such
> > programs, doesn't resolve CO-RE, etc, etc. You have to let
> > "prototypal" BPF program to be loaded before you can grab final
> > instructions. It's not great, but in your case it should work, right?
>
> hum, do we care? it should all be done when the 'new' program with
> the prologue is loaded, right?

yeah, you should care. If there is any BPF map involved, it is
properly resolved to correct FD (which is put into ldimm64 instruction
in BPF program code) during the load. If program is not autoloaded,
this is skipped. Same for any global variable or subprog call (if it's
not always inlined). So you very much should care for any non-trivial
program.

>
> I switched it off because the verifier failed to load the program
> without the prologue.. because in the original program there's no
> code to grab the arguments that the rest of the code depends on,
> so the verifier sees invalid access

Do you have an example of C code and corresponding BPF instructions
before/after prologue generation? Just curious to see in details how
this is done.

>
> >
> > > +
> > >         priv->need_prologue = true;
> > >         priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
> > >         if (!priv->insns_buf) {
> > > @@ -734,6 +762,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > >                 return -ENOMEM;
> > >         }
> > >
> >
> > [...]
> >
> > > +               /*
> > > +                * For each program that needs prologue we do following:
> > > +                *
> > > +                * - take its current instructions and use them
> > > +                *   to generate the new code with prologue
> > > +                *
> > > +                * - load new instructions with bpf_prog_load
> > > +                *   and keep the fd in proglogue_fds
> > > +                *
> > > +                * - new fd will be used bpf__foreach_event
> > > +                *   to connect this program with perf evsel
> > > +                */
> > > +               orig_insns = bpf_program__insns(prog);
> > > +               orig_insns_cnt = bpf_program__insn_cnt(prog);
> > > +
> > > +               pev = &priv->pev;
> > > +               for (i = 0; i < pev->ntevs; i++) {
> > > +                       err = preproc_gen_prologue(prog, i, orig_insns,
> > > +                                                  orig_insns_cnt, &res);
> > > +                       if (err)
> > > +                               return err;
> > > +
> > > +                       fd = bpf_prog_load(bpf_program__get_type(prog),
> >
> > nit: bpf_program__type() is preferred (we are deprecating/discouraging
> > "get_" prefixed getters in libbpf 1.0)
>
> ok, will change

It's been added in v0.7, yeah.

>
> >
> > > +                                          bpf_program__name(prog), "GPL",
> >
> > would it make sense to give each clone a distinct name?
>
> AFAICS the original code uses same prog name for instances,
> so I'd rather keep it that way
>

sure, np

> thanks,
> jirka
>
> >
> > > +                                          res.new_insn_ptr,
> > > +                                          res.new_insn_cnt, NULL);
> > > +                       if (fd < 0) {
> > > +                               char bf[128];
> > > +
> >
> > [...]

Re: [PATCH 3/3] perf tools: Rework prologue generation code

[Jiri Olsa]

On Fri, Feb 18, 2022 at 11:55:16AM -0800, Andrii Nakryiko wrote:
> On Fri, Feb 18, 2022 at 1:01 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> > > On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > Some functions we use now for bpf prologue generation are
> > > > going to be deprecated, so reworking the current code not
> > > > to use them.
> > > >
> > > > We need to replace following functions/struct:
> > > >    bpf_program__set_prep
> > > >    bpf_program__nth_fd
> > > >    struct bpf_prog_prep_result
> > > >
> > > > Current code uses bpf_program__set_prep to hook perf callback
> > > > before the program is loaded and provide new instructions with
> > > > the prologue.
> > > >
> > > > We workaround this by using objects's 'unloaded' programs instructions
> > > > for that specific program and load new ebpf programs with prologue
> > > > using separate bpf_prog_load calls.
> > > >
> > > > We keep new ebpf program instances descriptors in bpf programs
> > > > private struct.
> > > >
> > > > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > > >  1 file changed, 104 insertions(+), 18 deletions(-)
> > > >
> > >
> > > [...]
> > >
> > > >  errout:
> > > > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > >         struct bpf_prog_priv *priv = program_priv(prog);
> > > >         struct perf_probe_event *pev;
> > > >         bool need_prologue = false;
> > > > -       int err, i;
> > > > +       int i;
> > > >
> > > >         if (IS_ERR_OR_NULL(priv)) {
> > > >                 pr_debug("Internal error when hook preprocessor\n");
> > > > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > >                 return 0;
> > > >         }
> > > >
> > > > +       /*
> > > > +        * Do not load programs that need prologue, because we need
> > > > +        * to add prologue first, check bpf_object__load_prologue.
> > > > +        */
> > > > +       bpf_program__set_autoload(prog, false);
> > >
> > > if you set autoload to false, program instructions might be invalid in
> > > the end. Libbpf doesn't apply some (all?) relocations to such
> > > programs, doesn't resolve CO-RE, etc, etc. You have to let
> > > "prototypal" BPF program to be loaded before you can grab final
> > > instructions. It's not great, but in your case it should work, right?
> >
> > hum, do we care? it should all be done when the 'new' program with
> > the prologue is loaded, right?
> 
> yeah, you should care. If there is any BPF map involved, it is
> properly resolved to correct FD (which is put into ldimm64 instruction
> in BPF program code) during the load. If program is not autoloaded,
> this is skipped. Same for any global variable or subprog call (if it's
> not always inlined). So you very much should care for any non-trivial
> program.

ah too bad.. all that is in the load path, ok

> 
> >
> > I switched it off because the verifier failed to load the program
> > without the prologue.. because in the original program there's no
> > code to grab the arguments that the rest of the code depends on,
> > so the verifier sees invalid access
> 
> Do you have an example of C code and corresponding BPF instructions
> before/after prologue generation? Just curious to see in details how
> this is done.

so with following example:

	SEC("func=do_sched_setscheduler param->sched_priority@user")
	int bpf_func__setscheduler(void *ctx, int err, int param)
	{
		char fmt[] = "prio: %ld";
		bpf_trace_printk(fmt, sizeof(fmt), param);
		return 1;
	}

perf will attach the code to do_sched_setscheduler function,
and read 'param->sched_priority' into 'param' argument

so the resulting clang object expects 'param' to be in R3

	0000000000000000 <bpf_func__setscheduler>:
	       0:       b7 01 00 00 64 00 00 00 r1 = 100
	       1:       6b 1a f8 ff 00 00 00 00 *(u16 *)(r10 - 8) = r1
	       2:       18 01 00 00 70 72 69 6f 00 00 00 00 3a 20 25 6c r1 = 77926701655
	       4:       7b 1a f0 ff 00 00 00 00 *(u64 *)(r10 - 16) = r1
	       5:       bf a1 00 00 00 00 00 00 r1 = r10
	       6:       07 01 00 00 f0 ff ff ff r1 += -16
	       7:       b7 02 00 00 0a 00 00 00 r2 = 10
	       8:       85 00 00 00 06 00 00 00 call 6
	       9:       b7 00 00 00 01 00 00 00 r0 = 1
	      10:       95 00 00 00 00 00 00 00 exit

and R3 is loaded in the prologue code (first 15 instructions)
and it also sets 'err' (R2) with the result of the reading:

	   0: (bf) r6 = r1
	   1: (79) r3 = *(u64 *)(r6 +96)
	   2: (bf) r7 = r10
	   3: (07) r7 += -8
	   4: (7b) *(u64 *)(r10 -8) = r3
	   5: (b7) r2 = 8
	   6: (bf) r1 = r7
	   7: (85) call bpf_probe_read_user#-60848
	   8: (55) if r0 != 0x0 goto pc+2
	   9: (61) r3 = *(u32 *)(r10 -8)
	  10: (05) goto pc+3
	  11: (b7) r2 = 1
	  12: (b7) r3 = 0
	  13: (05) goto pc+1
	  14: (b7) r2 = 0
	  15: (bf) r1 = r6

	  16: (b7) r1 = 100
	  17: (6b) *(u16 *)(r10 -8) = r1
	  18: (18) r1 = 0x6c25203a6f697270
	  20: (7b) *(u64 *)(r10 -16) = r1
	  21: (bf) r1 = r10
	  22: (07) r1 += -16
	  23: (b7) r2 = 10
	  24: (85) call bpf_trace_printk#-54848
	  25: (b7) r0 = 1
	  26: (95) exit


I'm still scratching my head how to workaround this.. we do want maps
and all the other updates to the code, but verifier won't let it pass
without the prologue code

jirka

Re: [PATCH 3/3] perf tools: Rework prologue generation code

[Jiri Olsa]

On Sun, Feb 20, 2022 at 10:43:42AM -0800, Yonghong Song wrote:
> 
> 
> On 2/20/22 5:44 AM, Jiri Olsa wrote:
> > On Fri, Feb 18, 2022 at 11:55:16AM -0800, Andrii Nakryiko wrote:
> > > On Fri, Feb 18, 2022 at 1:01 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > > > 
> > > > On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> > > > > On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > > > 
> > > > > > Some functions we use now for bpf prologue generation are
> > > > > > going to be deprecated, so reworking the current code not
> > > > > > to use them.
> > > > > > 
> > > > > > We need to replace following functions/struct:
> > > > > >     bpf_program__set_prep
> > > > > >     bpf_program__nth_fd
> > > > > >     struct bpf_prog_prep_result
> > > > > > 
> > > > > > Current code uses bpf_program__set_prep to hook perf callback
> > > > > > before the program is loaded and provide new instructions with
> > > > > > the prologue.
> > > > > > 
> > > > > > We workaround this by using objects's 'unloaded' programs instructions
> > > > > > for that specific program and load new ebpf programs with prologue
> > > > > > using separate bpf_prog_load calls.
> > > > > > 
> > > > > > We keep new ebpf program instances descriptors in bpf programs
> > > > > > private struct.
> > > > > > 
> > > > > > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > > ---
> > > > > >   tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > > > > >   1 file changed, 104 insertions(+), 18 deletions(-)
> > > > > > 
> > > > > 
> > > > > [...]
> > > > > 
> > > > > >   errout:
> > > > > > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > > >          struct bpf_prog_priv *priv = program_priv(prog);
> > > > > >          struct perf_probe_event *pev;
> > > > > >          bool need_prologue = false;
> > > > > > -       int err, i;
> > > > > > +       int i;
> > > > > > 
> > > > > >          if (IS_ERR_OR_NULL(priv)) {
> > > > > >                  pr_debug("Internal error when hook preprocessor\n");
> > > > > > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > > >                  return 0;
> > > > > >          }
> > > > > > 
> > > > > > +       /*
> > > > > > +        * Do not load programs that need prologue, because we need
> > > > > > +        * to add prologue first, check bpf_object__load_prologue.
> > > > > > +        */
> > > > > > +       bpf_program__set_autoload(prog, false);
> > > > > 
> > > > > if you set autoload to false, program instructions might be invalid in
> > > > > the end. Libbpf doesn't apply some (all?) relocations to such
> > > > > programs, doesn't resolve CO-RE, etc, etc. You have to let
> > > > > "prototypal" BPF program to be loaded before you can grab final
> > > > > instructions. It's not great, but in your case it should work, right?
> > > > 
> > > > hum, do we care? it should all be done when the 'new' program with
> > > > the prologue is loaded, right?
> > > 
> > > yeah, you should care. If there is any BPF map involved, it is
> > > properly resolved to correct FD (which is put into ldimm64 instruction
> > > in BPF program code) during the load. If program is not autoloaded,
> > > this is skipped. Same for any global variable or subprog call (if it's
> > > not always inlined). So you very much should care for any non-trivial
> > > program.
> > 
> > ah too bad.. all that is in the load path, ok
> > 
> > > 
> > > > 
> > > > I switched it off because the verifier failed to load the program
> > > > without the prologue.. because in the original program there's no
> > > > code to grab the arguments that the rest of the code depends on,
> > > > so the verifier sees invalid access
> > > 
> > > Do you have an example of C code and corresponding BPF instructions
> > > before/after prologue generation? Just curious to see in details how
> > > this is done.
> > 
> > so with following example:
> > 
> > 	SEC("func=do_sched_setscheduler param->sched_priority@user")
> > 	int bpf_func__setscheduler(void *ctx, int err, int param)
> > 	{
> > 		char fmt[] = "prio: %ld";
> > 		bpf_trace_printk(fmt, sizeof(fmt), param);
> > 		return 1;
> > 	}
> > 
> > perf will attach the code to do_sched_setscheduler function,
> > and read 'param->sched_priority' into 'param' argument
> > 
> > so the resulting clang object expects 'param' to be in R3
> > 
> > 	0000000000000000 <bpf_func__setscheduler>:
> > 	       0:       b7 01 00 00 64 00 00 00 r1 = 100
> > 	       1:       6b 1a f8 ff 00 00 00 00 *(u16 *)(r10 - 8) = r1
> > 	       2:       18 01 00 00 70 72 69 6f 00 00 00 00 3a 20 25 6c r1 = 77926701655
> > 	       4:       7b 1a f0 ff 00 00 00 00 *(u64 *)(r10 - 16) = r1
> > 	       5:       bf a1 00 00 00 00 00 00 r1 = r10
> > 	       6:       07 01 00 00 f0 ff ff ff r1 += -16
> > 	       7:       b7 02 00 00 0a 00 00 00 r2 = 10
> > 	       8:       85 00 00 00 06 00 00 00 call 6
> > 	       9:       b7 00 00 00 01 00 00 00 r0 = 1
> > 	      10:       95 00 00 00 00 00 00 00 exit
> > 
> > and R3 is loaded in the prologue code (first 15 instructions)
> > and it also sets 'err' (R2) with the result of the reading:
> > 
> > 	   0: (bf) r6 = r1
> > 	   1: (79) r3 = *(u64 *)(r6 +96)
> > 	   2: (bf) r7 = r10
> > 	   3: (07) r7 += -8
> > 	   4: (7b) *(u64 *)(r10 -8) = r3
> > 	   5: (b7) r2 = 8
> > 	   6: (bf) r1 = r7
> > 	   7: (85) call bpf_probe_read_user#-60848
> > 	   8: (55) if r0 != 0x0 goto pc+2
> > 	   9: (61) r3 = *(u32 *)(r10 -8)
> > 	  10: (05) goto pc+3
> > 	  11: (b7) r2 = 1
> > 	  12: (b7) r3 = 0
> > 	  13: (05) goto pc+1
> > 	  14: (b7) r2 = 0
> > 	  15: (bf) r1 = r6
> > 
> > 	  16: (b7) r1 = 100
> > 	  17: (6b) *(u16 *)(r10 -8) = r1
> > 	  18: (18) r1 = 0x6c25203a6f697270
> > 	  20: (7b) *(u64 *)(r10 -16) = r1
> > 	  21: (bf) r1 = r10
> > 	  22: (07) r1 += -16
> > 	  23: (b7) r2 = 10
> > 	  24: (85) call bpf_trace_printk#-54848
> > 	  25: (b7) r0 = 1
> > 	  26: (95) exit
> 
> Just curious. Is the prologue code generated through C code or through
> asm code? Is it possible prologue code can be generated through C

it's C code in perf generating bpf instructions:
  https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/bpf-prologue.c?h=perf/core

> code with similar mechanism like BPF_PROG macro? Or this is already
> an API which cannot be changed?

do you mean to have some stub like:

  int bpf_func__setscheduler_stub(void *ctx)
  {
          return bpf_func__setscheduler(ctx, 0, 0)
  }

  int bpf_func__setscheduler(void *ctx, int err, int param)
  {
          char fmt[] = "prio: %ld";
          bpf_trace_printk(fmt, sizeof(fmt), param);
          return 1;
  }

to make verifier happy

then we'd need instructions for bpf_func__setscheduler

it looks like subprogram instructions are appended and we should
be able to locate bpf_func__setscheduler start in instructions
returned in bpf_program__insns ? anyway does not look nice ;-)

jirka

Re: [PATCH 3/3] perf tools: Rework prologue generation code

[Andrii Nakryiko]

On Sun, Feb 20, 2022 at 5:44 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Fri, Feb 18, 2022 at 11:55:16AM -0800, Andrii Nakryiko wrote:
> > On Fri, Feb 18, 2022 at 1:01 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> > > > On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > >
> > > > > Some functions we use now for bpf prologue generation are
> > > > > going to be deprecated, so reworking the current code not
> > > > > to use them.
> > > > >
> > > > > We need to replace following functions/struct:
> > > > >    bpf_program__set_prep
> > > > >    bpf_program__nth_fd
> > > > >    struct bpf_prog_prep_result
> > > > >
> > > > > Current code uses bpf_program__set_prep to hook perf callback
> > > > > before the program is loaded and provide new instructions with
> > > > > the prologue.
> > > > >
> > > > > We workaround this by using objects's 'unloaded' programs instructions
> > > > > for that specific program and load new ebpf programs with prologue
> > > > > using separate bpf_prog_load calls.
> > > > >
> > > > > We keep new ebpf program instances descriptors in bpf programs
> > > > > private struct.
> > > > >
> > > > > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > ---
> > > > >  tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > > > >  1 file changed, 104 insertions(+), 18 deletions(-)
> > > > >
> > > >
> > > > [...]
> > > >
> > > > >  errout:
> > > > > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > >         struct bpf_prog_priv *priv = program_priv(prog);
> > > > >         struct perf_probe_event *pev;
> > > > >         bool need_prologue = false;
> > > > > -       int err, i;
> > > > > +       int i;
> > > > >
> > > > >         if (IS_ERR_OR_NULL(priv)) {
> > > > >                 pr_debug("Internal error when hook preprocessor\n");
> > > > > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > >                 return 0;
> > > > >         }
> > > > >
> > > > > +       /*
> > > > > +        * Do not load programs that need prologue, because we need
> > > > > +        * to add prologue first, check bpf_object__load_prologue.
> > > > > +        */
> > > > > +       bpf_program__set_autoload(prog, false);
> > > >
> > > > if you set autoload to false, program instructions might be invalid in
> > > > the end. Libbpf doesn't apply some (all?) relocations to such
> > > > programs, doesn't resolve CO-RE, etc, etc. You have to let
> > > > "prototypal" BPF program to be loaded before you can grab final
> > > > instructions. It's not great, but in your case it should work, right?
> > >
> > > hum, do we care? it should all be done when the 'new' program with
> > > the prologue is loaded, right?
> >
> > yeah, you should care. If there is any BPF map involved, it is
> > properly resolved to correct FD (which is put into ldimm64 instruction
> > in BPF program code) during the load. If program is not autoloaded,
> > this is skipped. Same for any global variable or subprog call (if it's
> > not always inlined). So you very much should care for any non-trivial
> > program.
>
> ah too bad.. all that is in the load path, ok
>
> >
> > >
> > > I switched it off because the verifier failed to load the program
> > > without the prologue.. because in the original program there's no
> > > code to grab the arguments that the rest of the code depends on,
> > > so the verifier sees invalid access
> >
> > Do you have an example of C code and corresponding BPF instructions
> > before/after prologue generation? Just curious to see in details how
> > this is done.
>
> so with following example:
>
>         SEC("func=do_sched_setscheduler param->sched_priority@user")
>         int bpf_func__setscheduler(void *ctx, int err, int param)
>         {
>                 char fmt[] = "prio: %ld";
>                 bpf_trace_printk(fmt, sizeof(fmt), param);
>                 return 1;
>         }
>
> perf will attach the code to do_sched_setscheduler function,
> and read 'param->sched_priority' into 'param' argument
>
> so the resulting clang object expects 'param' to be in R3
>
>         0000000000000000 <bpf_func__setscheduler>:
>                0:       b7 01 00 00 64 00 00 00 r1 = 100
>                1:       6b 1a f8 ff 00 00 00 00 *(u16 *)(r10 - 8) = r1
>                2:       18 01 00 00 70 72 69 6f 00 00 00 00 3a 20 25 6c r1 = 77926701655
>                4:       7b 1a f0 ff 00 00 00 00 *(u64 *)(r10 - 16) = r1
>                5:       bf a1 00 00 00 00 00 00 r1 = r10
>                6:       07 01 00 00 f0 ff ff ff r1 += -16
>                7:       b7 02 00 00 0a 00 00 00 r2 = 10
>                8:       85 00 00 00 06 00 00 00 call 6
>                9:       b7 00 00 00 01 00 00 00 r0 = 1
>               10:       95 00 00 00 00 00 00 00 exit
>
> and R3 is loaded in the prologue code (first 15 instructions)
> and it also sets 'err' (R2) with the result of the reading:
>
>            0: (bf) r6 = r1
>            1: (79) r3 = *(u64 *)(r6 +96)
>            2: (bf) r7 = r10
>            3: (07) r7 += -8
>            4: (7b) *(u64 *)(r10 -8) = r3
>            5: (b7) r2 = 8
>            6: (bf) r1 = r7
>            7: (85) call bpf_probe_read_user#-60848
>            8: (55) if r0 != 0x0 goto pc+2
>            9: (61) r3 = *(u32 *)(r10 -8)
>           10: (05) goto pc+3
>           11: (b7) r2 = 1
>           12: (b7) r3 = 0
>           13: (05) goto pc+1
>           14: (b7) r2 = 0
>           15: (bf) r1 = r6
>
>           16: (b7) r1 = 100
>           17: (6b) *(u16 *)(r10 -8) = r1
>           18: (18) r1 = 0x6c25203a6f697270
>           20: (7b) *(u64 *)(r10 -16) = r1
>           21: (bf) r1 = r10
>           22: (07) r1 += -16
>           23: (b7) r2 = 10
>           24: (85) call bpf_trace_printk#-54848
>           25: (b7) r0 = 1
>           26: (95) exit
>
>
> I'm still scratching my head how to workaround this.. we do want maps
> and all the other updates to the code, but verifier won't let it pass
> without the prologue code

ugh, perf cornered itself into supporting this crazy scheme and now
there is no good solution. I'm still questioning the value of
supporting this going forward. Is there an evidence that anyone is
using this functionality at all? Is it worth it trying to carry it on
just because we have some example that exercises this feature?

Anyways, one way to solve this is to add bpf_program__set_insns() that
could be called from prog_init_fn callback (which I just realized
hasn't landed yet, I'll send v4 today) to prepend a simple preamble
like this:

r1 = 0;
r2 = 0;
r3 = 0;
f4 = 0;
r5 = 0; /* how many input arguments we support? */

This will make all input arguments initialized, libbpf will be able to
adjust all the relocations and stuff. Once this "prototype program" is
loaded, perf can grab final instructions and replace first X
instructions with desired preamble.

But... ugliness and horror, yeah :(


>
> jirka

Re: [PATCH 3/3] perf tools: Rework prologue generation code

[Jiri Olsa]

On Wed, Feb 23, 2022 at 02:29:56PM -0800, Andrii Nakryiko wrote:

SNIP

> > and R3 is loaded in the prologue code (first 15 instructions)
> > and it also sets 'err' (R2) with the result of the reading:
> >
> >            0: (bf) r6 = r1
> >            1: (79) r3 = *(u64 *)(r6 +96)
> >            2: (bf) r7 = r10
> >            3: (07) r7 += -8
> >            4: (7b) *(u64 *)(r10 -8) = r3
> >            5: (b7) r2 = 8
> >            6: (bf) r1 = r7
> >            7: (85) call bpf_probe_read_user#-60848
> >            8: (55) if r0 != 0x0 goto pc+2
> >            9: (61) r3 = *(u32 *)(r10 -8)
> >           10: (05) goto pc+3
> >           11: (b7) r2 = 1
> >           12: (b7) r3 = 0
> >           13: (05) goto pc+1
> >           14: (b7) r2 = 0
> >           15: (bf) r1 = r6
> >
> >           16: (b7) r1 = 100
> >           17: (6b) *(u16 *)(r10 -8) = r1
> >           18: (18) r1 = 0x6c25203a6f697270
> >           20: (7b) *(u64 *)(r10 -16) = r1
> >           21: (bf) r1 = r10
> >           22: (07) r1 += -16
> >           23: (b7) r2 = 10
> >           24: (85) call bpf_trace_printk#-54848
> >           25: (b7) r0 = 1
> >           26: (95) exit
> >
> >
> > I'm still scratching my head how to workaround this.. we do want maps
> > and all the other updates to the code, but verifier won't let it pass
> > without the prologue code
> 
> ugh, perf cornered itself into supporting this crazy scheme and now

well, it just used the interface that was provided at the time

> there is no good solution. I'm still questioning the value of
> supporting this going forward. Is there an evidence that anyone is
> using this functionality at all? Is it worth it trying to carry it on
> just because we have some example that exercises this feature?

yea we discussed this again and I think we can somehow mark this
feature in perf as deprecated and remove it after some time,
because even with the workaround below it'll be pita ;-)

or people will come and scream and we will find some other solution

I already sent the rest of the changes (prog/map priv) separately
and will send some RFC for the deprecation

thanks,
jirka

> 
> Anyways, one way to solve this is to add bpf_program__set_insns() that
> could be called from prog_init_fn callback (which I just realized
> hasn't landed yet, I'll send v4 today) to prepend a simple preamble
> like this:
> 
> r1 = 0;
> r2 = 0;
> r3 = 0;
> f4 = 0;
> r5 = 0; /* how many input arguments we support? */
> 
> This will make all input arguments initialized, libbpf will be able to
> adjust all the relocations and stuff. Once this "prototype program" is
> loaded, perf can grab final instructions and replace first X
> instructions with desired preamble.
> 
> But... ugliness and horror, yeah :(
> 
> 
> >
> > jirka

Re: [PATCH 3/3] perf tools: Rework prologue generation code

[Arnaldo Carvalho de Melo]

Em Fri, Feb 25, 2022 at 01:14:49PM +0100, Jiri Olsa escreveu:
> On Wed, Feb 23, 2022 at 02:29:56PM -0800, Andrii Nakryiko wrote:
> 
> SNIP
> 
> > > and R3 is loaded in the prologue code (first 15 instructions)
> > > and it also sets 'err' (R2) with the result of the reading:
> > >
> > >            0: (bf) r6 = r1
> > >            1: (79) r3 = *(u64 *)(r6 +96)
> > >            2: (bf) r7 = r10
> > >            3: (07) r7 += -8
> > >            4: (7b) *(u64 *)(r10 -8) = r3
> > >            5: (b7) r2 = 8
> > >            6: (bf) r1 = r7
> > >            7: (85) call bpf_probe_read_user#-60848
> > >            8: (55) if r0 != 0x0 goto pc+2
> > >            9: (61) r3 = *(u32 *)(r10 -8)
> > >           10: (05) goto pc+3
> > >           11: (b7) r2 = 1
> > >           12: (b7) r3 = 0
> > >           13: (05) goto pc+1
> > >           14: (b7) r2 = 0
> > >           15: (bf) r1 = r6
> > >
> > >           16: (b7) r1 = 100
> > >           17: (6b) *(u16 *)(r10 -8) = r1
> > >           18: (18) r1 = 0x6c25203a6f697270
> > >           20: (7b) *(u64 *)(r10 -16) = r1
> > >           21: (bf) r1 = r10
> > >           22: (07) r1 += -16
> > >           23: (b7) r2 = 10
> > >           24: (85) call bpf_trace_printk#-54848
> > >           25: (b7) r0 = 1
> > >           26: (95) exit
> > >
> > >
> > > I'm still scratching my head how to workaround this.. we do want maps
> > > and all the other updates to the code, but verifier won't let it pass
> > > without the prologue code
> > 
> > ugh, perf cornered itself into supporting this crazy scheme and now
 
> well, it just used the interface that was provided at the time

At the time it was where experimentation was done with tooling for eBPF,
Wangnan tried to provide a compact way to give access to parameters.

The problem now is for libbpf to remove something that is used and that
was documented to some extent in the perf tools examples so there _may_
be some usage of it, we just can't know.

Its like Linux removing some syscall that is "crazy" and wait for
somebody to complain of the breakage caused when they update to a new
version.
 
> > there is no good solution. I'm still questioning the value of
> > supporting this going forward. Is there an evidence that anyone is
> > using this functionality at all? Is it worth it trying to carry it on
> > just because we have some example that exercises this feature?
 
> yea we discussed this again and I think we can somehow mark this
> feature in perf as deprecated and remove it after some time,
> because even with the workaround below it'll be pita ;-)
> 
> or people will come and scream and we will find some other solution

:-\ if you have some "ugly" way to keep the feature, can't we go with
it?
 
> I already sent the rest of the changes (prog/map priv) separately
> and will send some RFC for the deprecation

I'll look at it now.

Thanks for your work on this, Jiri.

- Araldo
 
> thanks,
> jirka
> 
> > 
> > Anyways, one way to solve this is to add bpf_program__set_insns() that
> > could be called from prog_init_fn callback (which I just realized
> > hasn't landed yet, I'll send v4 today) to prepend a simple preamble
> > like this:
> > 
> > r1 = 0;
> > r2 = 0;
> > r3 = 0;
> > f4 = 0;
> > r5 = 0; /* how many input arguments we support? */
> > 
> > This will make all input arguments initialized, libbpf will be able to
> > adjust all the relocations and stuff. Once this "prototype program" is
> > loaded, perf can grab final instructions and replace first X
> > instructions with desired preamble.
> > 
> > But... ugliness and horror, yeah :(
> > 
> > 
> > >
> > > jirka

-- 

- Arnaldo