bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/2] perf: stop using deprecated bpf APIs
@ 2021-12-16 22:21 Christy Lee
  2021-12-16 22:21 ` [PATCH bpf-next 1/2] perf: stop using deprecated bpf_prog_load() API Christy Lee
  2021-12-16 22:21 ` [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API Christy Lee
  0 siblings, 2 replies; 18+ messages in thread
From: Christy Lee @ 2021-12-16 22:21 UTC (permalink / raw)
  To: andrii, acme
  Cc: christylee, christyc.y.lee, bpf, linux-perf-users, kernel-team

Except for multi-instance bpf program APIs, remove perf use cases
of deprecated bpf APIs.

Christy Lee (2):
  perf: stop using deprecated bpf_prog_load() API
  perf: stop using deprecated bpf__object_next() API

 tools/perf/tests/bpf.c       | 14 ++-----
 tools/perf/util/bpf-loader.c | 73 +++++++++++++++++++++++++++---------
 tools/perf/util/bpf-loader.h |  1 +
 3 files changed, 60 insertions(+), 28 deletions(-)

--
2.30.2

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

* [PATCH bpf-next 1/2] perf: stop using deprecated bpf_prog_load() API
  2021-12-16 22:21 [PATCH bpf-next 0/2] perf: stop using deprecated bpf APIs Christy Lee
@ 2021-12-16 22:21 ` Christy Lee
  2021-12-16 22:21 ` [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API Christy Lee
  1 sibling, 0 replies; 18+ messages in thread
From: Christy Lee @ 2021-12-16 22:21 UTC (permalink / raw)
  To: andrii, acme
  Cc: christylee, christyc.y.lee, bpf, linux-perf-users, kernel-team

Signed-off-by: Christy Lee <christylee@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/perf/tests/bpf.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/tools/perf/tests/bpf.c b/tools/perf/tests/bpf.c
index 573490530194..57b9591f7cbb 100644
--- a/tools/perf/tests/bpf.c
+++ b/tools/perf/tests/bpf.c
@@ -281,8 +281,8 @@ static int __test__bpf(int idx)
 
 static int check_env(void)
 {
+	LIBBPF_OPTS(bpf_prog_load_opts, opts);
 	int err;
-	unsigned int kver_int;
 	char license[] = "GPL";
 
 	struct bpf_insn insns[] = {
@@ -290,19 +290,13 @@ static int check_env(void)
 		BPF_EXIT_INSN(),
 	};
 
-	err = fetch_kernel_version(&kver_int, NULL, 0);
+	err = fetch_kernel_version(&opts.kern_version, NULL, 0);
 	if (err) {
 		pr_debug("Unable to get kernel version\n");
 		return err;
 	}
-
-/* temporarily disable libbpf deprecation warnings */
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
-	err = bpf_load_program(BPF_PROG_TYPE_KPROBE, insns,
-			       ARRAY_SIZE(insns),
-			       license, kver_int, NULL, 0);
-#pragma GCC diagnostic pop
+	err = bpf_prog_load(BPF_PROG_TYPE_KPROBE, NULL, license, insns,
+			    ARRAY_SIZE(insns), &opts);
 	if (err < 0) {
 		pr_err("Missing basic BPF support, skip this test: %s\n",
 		       strerror(errno));
-- 
2.30.2


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

* [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2021-12-16 22:21 [PATCH bpf-next 0/2] perf: stop using deprecated bpf APIs Christy Lee
  2021-12-16 22:21 ` [PATCH bpf-next 1/2] perf: stop using deprecated bpf_prog_load() API Christy Lee
@ 2021-12-16 22:21 ` Christy Lee
  2021-12-21  8:22   ` Jiri Olsa
  1 sibling, 1 reply; 18+ messages in thread
From: Christy Lee @ 2021-12-16 22:21 UTC (permalink / raw)
  To: andrii, acme
  Cc: christylee, christyc.y.lee, bpf, linux-perf-users, kernel-team

bpf__object_next is deprecated, track bpf_objects directly in
perf instead.

Signed-off-by: Christy Lee <christylee@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/perf/util/bpf-loader.c | 72 +++++++++++++++++++++++++++---------
 tools/perf/util/bpf-loader.h |  1 +
 2 files changed, 55 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index 528aeb0ab79d..9e3988fd719a 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -29,9 +29,6 @@
 
 #include <internal/xyarray.h>
 
-/* temporarily disable libbpf deprecation warnings */
-#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
-
 static int libbpf_perf_print(enum libbpf_print_level level __attribute__((unused)),
 			      const char *fmt, va_list args)
 {
@@ -49,6 +46,36 @@ struct bpf_prog_priv {
 	int *type_mapping;
 };
 
+struct bpf_perf_object {
+	struct bpf_object *obj;
+	struct list_head list;
+};
+
+static LIST_HEAD(bpf_objects_list);
+
+struct bpf_perf_object *bpf_perf_object__next(struct bpf_perf_object *prev)
+{
+	struct bpf_perf_object *next;
+
+	if (!prev)
+		next = list_first_entry(&bpf_objects_list,
+					struct bpf_perf_object, list);
+	else
+		next = list_next_entry(prev, list);
+
+	/* Empty list is noticed here so don't need checking on entry. */
+	if (&next->list == &bpf_objects_list)
+		return NULL;
+
+	return next;
+}
+
+#define bpf_perf_object__for_each(perf_obj, tmp, obj)                          \
+	for ((perf_obj) = bpf_perf_object__next(NULL),                         \
+	    (tmp) = bpf_perf_object__next(perf_obj), (obj) = NULL;             \
+	     (perf_obj) != NULL; (perf_obj) = (tmp),                           \
+	    (tmp) = bpf_perf_object__next(tmp), (obj) = (perf_obj)->obj)
+
 static bool libbpf_initialized;
 
 struct bpf_object *
@@ -113,9 +140,10 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
 
 void bpf__clear(void)
 {
-	struct bpf_object *obj, *tmp;
+	struct bpf_perf_object *perf_obj, *tmp;
+	struct bpf_object *obj;
 
-	bpf_object__for_each_safe(obj, tmp) {
+	bpf_perf_object__for_each(perf_obj, tmp, obj) {
 		bpf__unprobe(obj);
 		bpf_object__close(obj);
 	}
@@ -621,8 +649,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
 	if (err)
 		return err;
 
+/* temporarily disable libbpf deprecation warnings */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
 	err = bpf_program__set_prep(prog, priv->nr_types,
 				    preproc_gen_prologue);
+#pragma GCC diagnostic pop
 	return err;
 }
 
@@ -776,7 +808,11 @@ int bpf__foreach_event(struct bpf_object *obj,
 			if (priv->need_prologue) {
 				int type = priv->type_mapping[i];
 
+/* temporarily disable libbpf deprecation warnings */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
 				fd = bpf_program__nth_fd(prog, type);
+#pragma GCC diagnostic pop
 			} else {
 				fd = bpf_program__fd(prog);
 			}
@@ -1498,10 +1534,11 @@ apply_obj_config_object(struct bpf_object *obj)
 
 int bpf__apply_obj_config(void)
 {
-	struct bpf_object *obj, *tmp;
+	struct bpf_perf_object *perf_obj, *tmp;
+	struct bpf_object *obj;
 	int err;
 
-	bpf_object__for_each_safe(obj, tmp) {
+	bpf_perf_object__for_each(perf_obj, tmp, obj) {
 		err = apply_obj_config_object(obj);
 		if (err)
 			return err;
@@ -1510,26 +1547,25 @@ int bpf__apply_obj_config(void)
 	return 0;
 }
 
-#define bpf__for_each_map(pos, obj, objtmp)	\
-	bpf_object__for_each_safe(obj, objtmp)	\
-		bpf_object__for_each_map(pos, obj)
+#define bpf__perf_for_each_map(perf_obj, tmp, obj, map)                        \
+	bpf_perf_object__for_each(perf_obj, tmp, obj)                          \
+		bpf_object__for_each_map(map, obj)
 
-#define bpf__for_each_map_named(pos, obj, objtmp, name)	\
-	bpf__for_each_map(pos, obj, objtmp) 		\
-		if (bpf_map__name(pos) && 		\
-			(strcmp(name, 			\
-				bpf_map__name(pos)) == 0))
+#define bpf__perf_for_each_map_named(perf_obj, tmp, obj, map, name)            \
+	bpf__perf_for_each_map(perf_obj, tmp, obj, map)                        \
+		if (bpf_map__name(map) && (strcmp(name, bpf_map__name(map)) == 0))
 
 struct evsel *bpf__setup_output_event(struct evlist *evlist, const char *name)
 {
 	struct bpf_map_priv *tmpl_priv = NULL;
-	struct bpf_object *obj, *tmp;
+	struct bpf_perf_object *perf_obj, *tmp;
+	struct bpf_object *obj;
 	struct evsel *evsel = NULL;
 	struct bpf_map *map;
 	int err;
 	bool need_init = false;
 
-	bpf__for_each_map_named(map, obj, tmp, name) {
+	bpf__perf_for_each_map_named(perf_obj, tmp, obj, map, name) {
 		struct bpf_map_priv *priv = bpf_map__priv(map);
 
 		if (IS_ERR(priv))
@@ -1565,7 +1601,7 @@ struct evsel *bpf__setup_output_event(struct evlist *evlist, const char *name)
 		evsel = evlist__last(evlist);
 	}
 
-	bpf__for_each_map_named(map, obj, tmp, name) {
+	bpf__perf_for_each_map_named(perf_obj, tmp, obj, map, name) {
 		struct bpf_map_priv *priv = bpf_map__priv(map);
 
 		if (IS_ERR(priv))
diff --git a/tools/perf/util/bpf-loader.h b/tools/perf/util/bpf-loader.h
index 5d1c725cea29..95262b7e936f 100644
--- a/tools/perf/util/bpf-loader.h
+++ b/tools/perf/util/bpf-loader.h
@@ -53,6 +53,7 @@ typedef int (*bpf_prog_iter_callback_t)(const char *group, const char *event,
 
 #ifdef HAVE_LIBBPF_SUPPORT
 struct bpf_object *bpf__prepare_load(const char *filename, bool source);
+struct bpf_perf_object *bpf_perf_object__next(struct bpf_perf_object *prev);
 int bpf__strerror_prepare_load(const char *filename, bool source,
 			       int err, char *buf, size_t size);
 
-- 
2.30.2


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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2021-12-16 22:21 ` [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API Christy Lee
@ 2021-12-21  8:22   ` Jiri Olsa
  2021-12-21 21:58     ` Andrii Nakryiko
  0 siblings, 1 reply; 18+ messages in thread
From: Jiri Olsa @ 2021-12-21  8:22 UTC (permalink / raw)
  To: Christy Lee
  Cc: andrii, acme, christyc.y.lee, bpf, linux-perf-users, kernel-team

On Thu, Dec 16, 2021 at 02:21:08PM -0800, Christy Lee wrote:
> bpf__object_next is deprecated, track bpf_objects directly in
> perf instead.
> 
> Signed-off-by: Christy Lee <christylee@fb.com>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/perf/util/bpf-loader.c | 72 +++++++++++++++++++++++++++---------
>  tools/perf/util/bpf-loader.h |  1 +
>  2 files changed, 55 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index 528aeb0ab79d..9e3988fd719a 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -29,9 +29,6 @@
>  
>  #include <internal/xyarray.h>
>  
> -/* temporarily disable libbpf deprecation warnings */
> -#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> -
>  static int libbpf_perf_print(enum libbpf_print_level level __attribute__((unused)),
>  			      const char *fmt, va_list args)
>  {
> @@ -49,6 +46,36 @@ struct bpf_prog_priv {
>  	int *type_mapping;
>  };
>  
> +struct bpf_perf_object {
> +	struct bpf_object *obj;
> +	struct list_head list;
> +};
> +
> +static LIST_HEAD(bpf_objects_list);

hum, so this duplicates libbpf's bpf_objects_list,
how do objects get on this list?

could you please put more comments in changelog
and share how you tested this?

thanks,
jirka

> +
> +struct bpf_perf_object *bpf_perf_object__next(struct bpf_perf_object *prev)
> +{
> +	struct bpf_perf_object *next;
> +
> +	if (!prev)
> +		next = list_first_entry(&bpf_objects_list,
> +					struct bpf_perf_object, list);
> +	else
> +		next = list_next_entry(prev, list);
> +
> +	/* Empty list is noticed here so don't need checking on entry. */
> +	if (&next->list == &bpf_objects_list)
> +		return NULL;
> +
> +	return next;
> +}
> +
> +#define bpf_perf_object__for_each(perf_obj, tmp, obj)                          \
> +	for ((perf_obj) = bpf_perf_object__next(NULL),                         \
> +	    (tmp) = bpf_perf_object__next(perf_obj), (obj) = NULL;             \
> +	     (perf_obj) != NULL; (perf_obj) = (tmp),                           \
> +	    (tmp) = bpf_perf_object__next(tmp), (obj) = (perf_obj)->obj)
> +
>  static bool libbpf_initialized;
>  
>  struct bpf_object *
> @@ -113,9 +140,10 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
>  
>  void bpf__clear(void)
>  {
> -	struct bpf_object *obj, *tmp;
> +	struct bpf_perf_object *perf_obj, *tmp;
> +	struct bpf_object *obj;
>  
> -	bpf_object__for_each_safe(obj, tmp) {
> +	bpf_perf_object__for_each(perf_obj, tmp, obj) {
>  		bpf__unprobe(obj);
>  		bpf_object__close(obj);
>  	}
> @@ -621,8 +649,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
>  	if (err)
>  		return err;
>  
> +/* temporarily disable libbpf deprecation warnings */
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
>  	err = bpf_program__set_prep(prog, priv->nr_types,
>  				    preproc_gen_prologue);
> +#pragma GCC diagnostic pop
>  	return err;
>  }
>  
> @@ -776,7 +808,11 @@ int bpf__foreach_event(struct bpf_object *obj,
>  			if (priv->need_prologue) {
>  				int type = priv->type_mapping[i];
>  
> +/* temporarily disable libbpf deprecation warnings */
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
>  				fd = bpf_program__nth_fd(prog, type);
> +#pragma GCC diagnostic pop
>  			} else {
>  				fd = bpf_program__fd(prog);
>  			}
> @@ -1498,10 +1534,11 @@ apply_obj_config_object(struct bpf_object *obj)
>  
>  int bpf__apply_obj_config(void)
>  {
> -	struct bpf_object *obj, *tmp;
> +	struct bpf_perf_object *perf_obj, *tmp;
> +	struct bpf_object *obj;
>  	int err;
>  
> -	bpf_object__for_each_safe(obj, tmp) {
> +	bpf_perf_object__for_each(perf_obj, tmp, obj) {
>  		err = apply_obj_config_object(obj);
>  		if (err)
>  			return err;
> @@ -1510,26 +1547,25 @@ int bpf__apply_obj_config(void)
>  	return 0;
>  }
>  
> -#define bpf__for_each_map(pos, obj, objtmp)	\
> -	bpf_object__for_each_safe(obj, objtmp)	\
> -		bpf_object__for_each_map(pos, obj)
> +#define bpf__perf_for_each_map(perf_obj, tmp, obj, map)                        \
> +	bpf_perf_object__for_each(perf_obj, tmp, obj)                          \
> +		bpf_object__for_each_map(map, obj)
>  
> -#define bpf__for_each_map_named(pos, obj, objtmp, name)	\
> -	bpf__for_each_map(pos, obj, objtmp) 		\
> -		if (bpf_map__name(pos) && 		\
> -			(strcmp(name, 			\
> -				bpf_map__name(pos)) == 0))
> +#define bpf__perf_for_each_map_named(perf_obj, tmp, obj, map, name)            \
> +	bpf__perf_for_each_map(perf_obj, tmp, obj, map)                        \
> +		if (bpf_map__name(map) && (strcmp(name, bpf_map__name(map)) == 0))
>  
>  struct evsel *bpf__setup_output_event(struct evlist *evlist, const char *name)
>  {
>  	struct bpf_map_priv *tmpl_priv = NULL;
> -	struct bpf_object *obj, *tmp;
> +	struct bpf_perf_object *perf_obj, *tmp;
> +	struct bpf_object *obj;
>  	struct evsel *evsel = NULL;
>  	struct bpf_map *map;
>  	int err;
>  	bool need_init = false;
>  
> -	bpf__for_each_map_named(map, obj, tmp, name) {
> +	bpf__perf_for_each_map_named(perf_obj, tmp, obj, map, name) {
>  		struct bpf_map_priv *priv = bpf_map__priv(map);
>  
>  		if (IS_ERR(priv))
> @@ -1565,7 +1601,7 @@ struct evsel *bpf__setup_output_event(struct evlist *evlist, const char *name)
>  		evsel = evlist__last(evlist);
>  	}
>  
> -	bpf__for_each_map_named(map, obj, tmp, name) {
> +	bpf__perf_for_each_map_named(perf_obj, tmp, obj, map, name) {
>  		struct bpf_map_priv *priv = bpf_map__priv(map);
>  
>  		if (IS_ERR(priv))
> diff --git a/tools/perf/util/bpf-loader.h b/tools/perf/util/bpf-loader.h
> index 5d1c725cea29..95262b7e936f 100644
> --- a/tools/perf/util/bpf-loader.h
> +++ b/tools/perf/util/bpf-loader.h
> @@ -53,6 +53,7 @@ typedef int (*bpf_prog_iter_callback_t)(const char *group, const char *event,
>  
>  #ifdef HAVE_LIBBPF_SUPPORT
>  struct bpf_object *bpf__prepare_load(const char *filename, bool source);
> +struct bpf_perf_object *bpf_perf_object__next(struct bpf_perf_object *prev);
>  int bpf__strerror_prepare_load(const char *filename, bool source,
>  			       int err, char *buf, size_t size);
>  
> -- 
> 2.30.2
> 


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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2021-12-21  8:22   ` Jiri Olsa
@ 2021-12-21 21:58     ` Andrii Nakryiko
  2021-12-22 13:44       ` Jiri Olsa
  0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2021-12-21 21:58 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Christy Lee, Andrii Nakryiko, Arnaldo Carvalho de Melo,
	Christy Lee, bpf, linux-perf-use.,
	Kernel Team

On Tue, Dec 21, 2021 at 12:23 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Dec 16, 2021 at 02:21:08PM -0800, Christy Lee wrote:
> > bpf__object_next is deprecated, track bpf_objects directly in
> > perf instead.
> >
> > Signed-off-by: Christy Lee <christylee@fb.com>
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  tools/perf/util/bpf-loader.c | 72 +++++++++++++++++++++++++++---------
> >  tools/perf/util/bpf-loader.h |  1 +
> >  2 files changed, 55 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> > index 528aeb0ab79d..9e3988fd719a 100644
> > --- a/tools/perf/util/bpf-loader.c
> > +++ b/tools/perf/util/bpf-loader.c
> > @@ -29,9 +29,6 @@
> >
> >  #include <internal/xyarray.h>
> >
> > -/* temporarily disable libbpf deprecation warnings */
> > -#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> > -
> >  static int libbpf_perf_print(enum libbpf_print_level level __attribute__((unused)),
> >                             const char *fmt, va_list args)
> >  {
> > @@ -49,6 +46,36 @@ struct bpf_prog_priv {
> >       int *type_mapping;
> >  };
> >
> > +struct bpf_perf_object {
> > +     struct bpf_object *obj;
> > +     struct list_head list;
> > +};
> > +
> > +static LIST_HEAD(bpf_objects_list);
>
> hum, so this duplicates libbpf's bpf_objects_list,
> how do objects get on this list?

yep, this list needs to be updated on perf side each time
bpf_object__open() (and any variant of open) is called.

>
> could you please put more comments in changelog
> and share how you tested this?

I actually have no idea how to test this as well, can you please share
some ideas?


BTW, while we are at it, Jiri, do you have any good ideas on how to
remove perf's usage of bpf_program__set_priv() and
bpf_program__set_prep() APIs in perf code base? These APIs are
deprecated as well, but seems like perf relies on them pretty heavily.
What would be the best way to stop using them?

For set_priv(), I think it should be totally fine to maintain a
separate lookup hash table by `struct bpf_program *` or its name, that
shouldn't be hard.

But for set_prep(), what does perf use it for? I assume for cloning
BPF programs, right? Anything else besides that? If it's just for
cloning, libbpf provides bpf_program__insns() API to get access to
underlying bpf_insn array, do you think it's possible to switch perf
to that instead?



>
> thanks,
> jirka
>
> > +
> > +struct bpf_perf_object *bpf_perf_object__next(struct bpf_perf_object *prev)
> > +{
> > +     struct bpf_perf_object *next;
> > +
> > +     if (!prev)
> > +             next = list_first_entry(&bpf_objects_list,
> > +                                     struct bpf_perf_object, list);
> > +     else
> > +             next = list_next_entry(prev, list);
> > +
> > +     /* Empty list is noticed here so don't need checking on entry. */
> > +     if (&next->list == &bpf_objects_list)
> > +             return NULL;
> > +
> > +     return next;
> > +}
> > +
> > +#define bpf_perf_object__for_each(perf_obj, tmp, obj)                          \
> > +     for ((perf_obj) = bpf_perf_object__next(NULL),                         \
> > +         (tmp) = bpf_perf_object__next(perf_obj), (obj) = NULL;             \
> > +          (perf_obj) != NULL; (perf_obj) = (tmp),                           \
> > +         (tmp) = bpf_perf_object__next(tmp), (obj) = (perf_obj)->obj)
> > +
> >  static bool libbpf_initialized;
> >
> >  struct bpf_object *
> > @@ -113,9 +140,10 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
> >
> >  void bpf__clear(void)
> >  {
> > -     struct bpf_object *obj, *tmp;
> > +     struct bpf_perf_object *perf_obj, *tmp;
> > +     struct bpf_object *obj;
> >
> > -     bpf_object__for_each_safe(obj, tmp) {
> > +     bpf_perf_object__for_each(perf_obj, tmp, obj) {
> >               bpf__unprobe(obj);
> >               bpf_object__close(obj);
> >       }
> > @@ -621,8 +649,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> >       if (err)
> >               return err;
> >
> > +/* temporarily disable libbpf deprecation warnings */
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> >       err = bpf_program__set_prep(prog, priv->nr_types,
> >                                   preproc_gen_prologue);
> > +#pragma GCC diagnostic pop
> >       return err;
> >  }
> >
> > @@ -776,7 +808,11 @@ int bpf__foreach_event(struct bpf_object *obj,
> >                       if (priv->need_prologue) {
> >                               int type = priv->type_mapping[i];
> >
> > +/* temporarily disable libbpf deprecation warnings */
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> >                               fd = bpf_program__nth_fd(prog, type);
> > +#pragma GCC diagnostic pop
> >                       } else {
> >                               fd = bpf_program__fd(prog);
> >                       }
> > @@ -1498,10 +1534,11 @@ apply_obj_config_object(struct bpf_object *obj)
> >
> >  int bpf__apply_obj_config(void)
> >  {
> > -     struct bpf_object *obj, *tmp;
> > +     struct bpf_perf_object *perf_obj, *tmp;
> > +     struct bpf_object *obj;
> >       int err;
> >
> > -     bpf_object__for_each_safe(obj, tmp) {
> > +     bpf_perf_object__for_each(perf_obj, tmp, obj) {
> >               err = apply_obj_config_object(obj);
> >               if (err)
> >                       return err;
> > @@ -1510,26 +1547,25 @@ int bpf__apply_obj_config(void)
> >       return 0;
> >  }
> >
> > -#define bpf__for_each_map(pos, obj, objtmp)  \
> > -     bpf_object__for_each_safe(obj, objtmp)  \
> > -             bpf_object__for_each_map(pos, obj)
> > +#define bpf__perf_for_each_map(perf_obj, tmp, obj, map)                        \
> > +     bpf_perf_object__for_each(perf_obj, tmp, obj)                          \
> > +             bpf_object__for_each_map(map, obj)
> >
> > -#define bpf__for_each_map_named(pos, obj, objtmp, name)      \
> > -     bpf__for_each_map(pos, obj, objtmp)             \
> > -             if (bpf_map__name(pos) &&               \
> > -                     (strcmp(name,                   \
> > -                             bpf_map__name(pos)) == 0))
> > +#define bpf__perf_for_each_map_named(perf_obj, tmp, obj, map, name)            \
> > +     bpf__perf_for_each_map(perf_obj, tmp, obj, map)                        \
> > +             if (bpf_map__name(map) && (strcmp(name, bpf_map__name(map)) == 0))
> >
> >  struct evsel *bpf__setup_output_event(struct evlist *evlist, const char *name)
> >  {
> >       struct bpf_map_priv *tmpl_priv = NULL;
> > -     struct bpf_object *obj, *tmp;
> > +     struct bpf_perf_object *perf_obj, *tmp;
> > +     struct bpf_object *obj;
> >       struct evsel *evsel = NULL;
> >       struct bpf_map *map;
> >       int err;
> >       bool need_init = false;
> >
> > -     bpf__for_each_map_named(map, obj, tmp, name) {
> > +     bpf__perf_for_each_map_named(perf_obj, tmp, obj, map, name) {
> >               struct bpf_map_priv *priv = bpf_map__priv(map);
> >
> >               if (IS_ERR(priv))
> > @@ -1565,7 +1601,7 @@ struct evsel *bpf__setup_output_event(struct evlist *evlist, const char *name)
> >               evsel = evlist__last(evlist);
> >       }
> >
> > -     bpf__for_each_map_named(map, obj, tmp, name) {
> > +     bpf__perf_for_each_map_named(perf_obj, tmp, obj, map, name) {
> >               struct bpf_map_priv *priv = bpf_map__priv(map);
> >
> >               if (IS_ERR(priv))
> > diff --git a/tools/perf/util/bpf-loader.h b/tools/perf/util/bpf-loader.h
> > index 5d1c725cea29..95262b7e936f 100644
> > --- a/tools/perf/util/bpf-loader.h
> > +++ b/tools/perf/util/bpf-loader.h
> > @@ -53,6 +53,7 @@ typedef int (*bpf_prog_iter_callback_t)(const char *group, const char *event,
> >
> >  #ifdef HAVE_LIBBPF_SUPPORT
> >  struct bpf_object *bpf__prepare_load(const char *filename, bool source);
> > +struct bpf_perf_object *bpf_perf_object__next(struct bpf_perf_object *prev);
> >  int bpf__strerror_prepare_load(const char *filename, bool source,
> >                              int err, char *buf, size_t size);
> >
> > --
> > 2.30.2
> >
>

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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2021-12-21 21:58     ` Andrii Nakryiko
@ 2021-12-22 13:44       ` Jiri Olsa
  2021-12-22 22:17         ` Andrii Nakryiko
  0 siblings, 1 reply; 18+ messages in thread
From: Jiri Olsa @ 2021-12-22 13:44 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Christy Lee, Andrii Nakryiko, Arnaldo Carvalho de Melo,
	Christy Lee, bpf, linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

On Tue, Dec 21, 2021 at 01:58:14PM -0800, Andrii Nakryiko wrote:
> On Tue, Dec 21, 2021 at 12:23 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Thu, Dec 16, 2021 at 02:21:08PM -0800, Christy Lee wrote:
> > > bpf__object_next is deprecated, track bpf_objects directly in
> > > perf instead.
> > >
> > > Signed-off-by: Christy Lee <christylee@fb.com>
> > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > >  tools/perf/util/bpf-loader.c | 72 +++++++++++++++++++++++++++---------
> > >  tools/perf/util/bpf-loader.h |  1 +
> > >  2 files changed, 55 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> > > index 528aeb0ab79d..9e3988fd719a 100644
> > > --- a/tools/perf/util/bpf-loader.c
> > > +++ b/tools/perf/util/bpf-loader.c
> > > @@ -29,9 +29,6 @@
> > >
> > >  #include <internal/xyarray.h>
> > >
> > > -/* temporarily disable libbpf deprecation warnings */
> > > -#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> > > -
> > >  static int libbpf_perf_print(enum libbpf_print_level level __attribute__((unused)),
> > >                             const char *fmt, va_list args)
> > >  {
> > > @@ -49,6 +46,36 @@ struct bpf_prog_priv {
> > >       int *type_mapping;
> > >  };
> > >
> > > +struct bpf_perf_object {
> > > +     struct bpf_object *obj;
> > > +     struct list_head list;
> > > +};
> > > +
> > > +static LIST_HEAD(bpf_objects_list);
> >
> > hum, so this duplicates libbpf's bpf_objects_list,
> > how do objects get on this list?
> 
> yep, this list needs to be updated on perf side each time
> bpf_object__open() (and any variant of open) is called.
> 
> >
> > could you please put more comments in changelog
> > and share how you tested this?
> 
> I actually have no idea how to test this as well, can you please share
> some ideas?
> 

I don't use it, I just know it's there.. that's why I asked ;-)

it's possible to specify bpf program on the perf command line
to be attached to event, like:

      # cat tools/perf/examples/bpf/hello.c
      #include <stdio.h>
    
      int syscall_enter(openat)(void *args)
      {
              puts("Hello, world\n");
              return 0;
      }
    
      license(GPL);
      #
      # perf trace -e openat,tools/perf/examples/bpf/hello.c cat /etc/passwd > /dev/null
         0.016 (         ): __bpf_stdout__:Hello, world
         0.018 ( 0.010 ms): cat/9079 openat(dfd: CWD, filename: /etc/ld.so.cache, flags: CLOEXEC) = 3
         0.057 (         ): __bpf_stdout__:Hello, world
         0.059 ( 0.011 ms): cat/9079 openat(dfd: CWD, filename: /lib64/libc.so.6, flags: CLOEXEC) = 3
         0.417 (         ): __bpf_stdout__:Hello, world
         0.419 ( 0.009 ms): cat/9079 openat(dfd: CWD, filename: /etc/passwd) = 3
      #

I took that example from commit message

> 
> BTW, while we are at it, Jiri, do you have any good ideas on how to
> remove perf's usage of bpf_program__set_priv() and
> bpf_program__set_prep() APIs in perf code base? These APIs are
> deprecated as well, but seems like perf relies on them pretty heavily.
> What would be the best way to stop using them?
> 
> For set_priv(), I think it should be totally fine to maintain a
> separate lookup hash table by `struct bpf_program *` or its name, that
> shouldn't be hard.

ok, so there's no alternative api for this one then

> 
> But for set_prep(), what does perf use it for? I assume for cloning
> BPF programs, right? Anything else besides that? If it's just for
> cloning, libbpf provides bpf_program__insns() API to get access to
> underlying bpf_insn array, do you think it's possible to switch perf
> to that instead?

look like it's used to generate prologs for programs:
  a08357d8dc7d perf bpf: Generate prologue for BPF programs

the author Wang Nan haven't touched that for some time,
I'm cc-ing other folks that were involved..  Arnaldo? ;-)

if nobody volunteers, I can check on that

jirka


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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2021-12-22 13:44       ` Jiri Olsa
@ 2021-12-22 22:17         ` Andrii Nakryiko
  2021-12-29 19:01           ` Christy Lee
  0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2021-12-22 22:17 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Christy Lee, Andrii Nakryiko, Arnaldo Carvalho de Melo,
	Christy Lee, bpf, linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

On Wed, Dec 22, 2021 at 5:44 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Dec 21, 2021 at 01:58:14PM -0800, Andrii Nakryiko wrote:
> > On Tue, Dec 21, 2021 at 12:23 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Thu, Dec 16, 2021 at 02:21:08PM -0800, Christy Lee wrote:
> > > > bpf__object_next is deprecated, track bpf_objects directly in
> > > > perf instead.
> > > >
> > > > Signed-off-by: Christy Lee <christylee@fb.com>
> > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > > ---
> > > >  tools/perf/util/bpf-loader.c | 72 +++++++++++++++++++++++++++---------
> > > >  tools/perf/util/bpf-loader.h |  1 +
> > > >  2 files changed, 55 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> > > > index 528aeb0ab79d..9e3988fd719a 100644
> > > > --- a/tools/perf/util/bpf-loader.c
> > > > +++ b/tools/perf/util/bpf-loader.c
> > > > @@ -29,9 +29,6 @@
> > > >
> > > >  #include <internal/xyarray.h>
> > > >
> > > > -/* temporarily disable libbpf deprecation warnings */
> > > > -#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> > > > -
> > > >  static int libbpf_perf_print(enum libbpf_print_level level __attribute__((unused)),
> > > >                             const char *fmt, va_list args)
> > > >  {
> > > > @@ -49,6 +46,36 @@ struct bpf_prog_priv {
> > > >       int *type_mapping;
> > > >  };
> > > >
> > > > +struct bpf_perf_object {
> > > > +     struct bpf_object *obj;
> > > > +     struct list_head list;
> > > > +};
> > > > +
> > > > +static LIST_HEAD(bpf_objects_list);
> > >
> > > hum, so this duplicates libbpf's bpf_objects_list,
> > > how do objects get on this list?
> >
> > yep, this list needs to be updated on perf side each time
> > bpf_object__open() (and any variant of open) is called.
> >
> > >
> > > could you please put more comments in changelog
> > > and share how you tested this?
> >
> > I actually have no idea how to test this as well, can you please share
> > some ideas?
> >
>
> I don't use it, I just know it's there.. that's why I asked ;-)
>
> it's possible to specify bpf program on the perf command line
> to be attached to event, like:
>
>       # cat tools/perf/examples/bpf/hello.c
>       #include <stdio.h>
>
>       int syscall_enter(openat)(void *args)
>       {
>               puts("Hello, world\n");
>               return 0;
>       }
>
>       license(GPL);
>       #
>       # perf trace -e openat,tools/perf/examples/bpf/hello.c cat /etc/passwd > /dev/null
>          0.016 (         ): __bpf_stdout__:Hello, world
>          0.018 ( 0.010 ms): cat/9079 openat(dfd: CWD, filename: /etc/ld.so.cache, flags: CLOEXEC) = 3
>          0.057 (         ): __bpf_stdout__:Hello, world
>          0.059 ( 0.011 ms): cat/9079 openat(dfd: CWD, filename: /lib64/libc.so.6, flags: CLOEXEC) = 3
>          0.417 (         ): __bpf_stdout__:Hello, world
>          0.419 ( 0.009 ms): cat/9079 openat(dfd: CWD, filename: /etc/passwd) = 3
>       #
>
> I took that example from commit message
>
> >
> > BTW, while we are at it, Jiri, do you have any good ideas on how to
> > remove perf's usage of bpf_program__set_priv() and
> > bpf_program__set_prep() APIs in perf code base? These APIs are
> > deprecated as well, but seems like perf relies on them pretty heavily.
> > What would be the best way to stop using them?
> >
> > For set_priv(), I think it should be totally fine to maintain a
> > separate lookup hash table by `struct bpf_program *` or its name, that
> > shouldn't be hard.
>
> ok, so there's no alternative api for this one then

nope

>
> >
> > But for set_prep(), what does perf use it for? I assume for cloning
> > BPF programs, right? Anything else besides that? If it's just for
> > cloning, libbpf provides bpf_program__insns() API to get access to
> > underlying bpf_insn array, do you think it's possible to switch perf
> > to that instead?
>
> look like it's used to generate prologs for programs:
>   a08357d8dc7d perf bpf: Generate prologue for BPF programs
>
> the author Wang Nan haven't touched that for some time,
> I'm cc-ing other folks that were involved..  Arnaldo? ;-)
>
> if nobody volunteers, I can check on that

thanks! I appreciate the help with moving perf away for libbpf's
obscure and deprecated APIs.

>
> jirka
>

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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2021-12-22 22:17         ` Andrii Nakryiko
@ 2021-12-29 19:01           ` Christy Lee
  2022-01-04 14:40             ` Jiri Olsa
  0 siblings, 1 reply; 18+ messages in thread
From: Christy Lee @ 2021-12-29 19:01 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jiri Olsa, Christy Lee, Andrii Nakryiko,
	Arnaldo Carvalho de Melo, bpf, linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

On Wed, Dec 22, 2021 at 2:17 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Dec 22, 2021 at 5:44 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Tue, Dec 21, 2021 at 01:58:14PM -0800, Andrii Nakryiko wrote:
> > > On Tue, Dec 21, 2021 at 12:23 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > > >
> > > > On Thu, Dec 16, 2021 at 02:21:08PM -0800, Christy Lee wrote:
> > > > > bpf__object_next is deprecated, track bpf_objects directly in
> > > > > perf instead.
> > > > >
> > > > > Signed-off-by: Christy Lee <christylee@fb.com>
> > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > > > ---
> > > > >  tools/perf/util/bpf-loader.c | 72 +++++++++++++++++++++++++++---------
> > > > >  tools/perf/util/bpf-loader.h |  1 +
> > > > >  2 files changed, 55 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> > > > > index 528aeb0ab79d..9e3988fd719a 100644
> > > > > --- a/tools/perf/util/bpf-loader.c
> > > > > +++ b/tools/perf/util/bpf-loader.c
> > > > > @@ -29,9 +29,6 @@
> > > > >
> > > > >  #include <internal/xyarray.h>
> > > > >
> > > > > -/* temporarily disable libbpf deprecation warnings */
> > > > > -#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> > > > > -
> > > > >  static int libbpf_perf_print(enum libbpf_print_level level __attribute__((unused)),
> > > > >                             const char *fmt, va_list args)
> > > > >  {
> > > > > @@ -49,6 +46,36 @@ struct bpf_prog_priv {
> > > > >       int *type_mapping;
> > > > >  };
> > > > >
> > > > > +struct bpf_perf_object {
> > > > > +     struct bpf_object *obj;
> > > > > +     struct list_head list;
> > > > > +};
> > > > > +
> > > > > +static LIST_HEAD(bpf_objects_list);
> > > >
> > > > hum, so this duplicates libbpf's bpf_objects_list,
> > > > how do objects get on this list?
> > >
> > > yep, this list needs to be updated on perf side each time
> > > bpf_object__open() (and any variant of open) is called.
> > >
> > > >
> > > > could you please put more comments in changelog
> > > > and share how you tested this?
> > >
> > > I actually have no idea how to test this as well, can you please share
> > > some ideas?
> > >
> >
> > I don't use it, I just know it's there.. that's why I asked ;-)
> >
> > it's possible to specify bpf program on the perf command line
> > to be attached to event, like:
> >
> >       # cat tools/perf/examples/bpf/hello.c
> >       #include <stdio.h>
> >
> >       int syscall_enter(openat)(void *args)
> >       {
> >               puts("Hello, world\n");
> >               return 0;
> >       }
> >
> >       license(GPL);
> >       #
> >       # perf trace -e openat,tools/perf/examples/bpf/hello.c cat /etc/passwd > /dev/null
> >          0.016 (         ): __bpf_stdout__:Hello, world
> >          0.018 ( 0.010 ms): cat/9079 openat(dfd: CWD, filename: /etc/ld.so.cache, flags: CLOEXEC) = 3
> >          0.057 (         ): __bpf_stdout__:Hello, world
> >          0.059 ( 0.011 ms): cat/9079 openat(dfd: CWD, filename: /lib64/libc.so.6, flags: CLOEXEC) = 3
> >          0.417 (         ): __bpf_stdout__:Hello, world
> >          0.419 ( 0.009 ms): cat/9079 openat(dfd: CWD, filename: /etc/passwd) = 3
> >       #
> >
> > I took that example from commit message
[...]

I found the original commit aa3abf30bb28addcf593578d37447d42e3f65fc3
that included a test case, but I'm having trouble reproducing it due to syntax
error. I am running this on bpf-next master without my patches.

I ran 'perf test -v LLVM' and used it's output to generate a script for
compiling the perf test object:

--------------------------------------------------
$ cat ~/bin/hello-ebpf
INPUT_FILE=/tmp/test.c
OUTPUT_FILE=/tmp/test.o

export KBUILD_DIR=/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build
export NR_CPUS=56
export LINUX_VERSION_CODE=0x50c00
export CLANG_EXEC=/data/users/christylee/devtools/llvm/latest/bin/clang
export CLANG_OPTIONS=-xc
export KERNEL_INC_OPTIONS="-nostdinc -isystem
/data/users/christylee/devtools/gcc/10.3.0/lib/gcc/x86_64-pc-linux-gnu/10.3.0/include
-I./arch/\
x86/include -I./arch/x86/include/generated  -I./include
-I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi
-I./include/uapi -I./in\
clude/generated/uapi -include ./include/linux/compiler-version.h
-include ./include/linux/kconfig.h"
export PERF_BPF_INC_OPTIONS=-I/home/christylee/lib/perf/include/bpf
export WORKING_DIR=/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build
export CLANG_SOURCE=-

rm -f $OUTPUT_FILE
cat $INPUT_FILE |
/data/users/christylee/devtools/llvm/latest/bin/clang -D__KERNEL__
-D__NR_CPUS__=56 -DLINUX_VERSION_CODE=0x50c00 -xc  -I/ho\
me/christylee/lib/perf/include/bpf  -nostdinc -isystem
/data/users/christylee/devtools/gcc/10.3.0/lib/gcc/x86_64-pc-linux-gnu/10.3.0/include
\
-I./arch/x86/include -I./arch/x86/include/generated  -I./include
-I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi
-I./include/ua\
pi -I./include/generated/uapi -include
./include/linux/compiler-version.h -include ./include/linux/kconfig.h
-Wno-unused-value -Wno-pointer-\
sign -working-directory
/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build -c - -target bpf
-O2 -o $OUTPUT_FILE
--------------------------------------------------

I then wrote and compiled a script that ask to get asks to put a probe
at a function that
does not exists in the kernel, it errors out as expected:

$ cat /tmp/test.c
__attribute__((section("fork=does_not_exist"), used)) int fork(void *ctx) {
    return 0;
}

char _license[] __attribute__((section("license"), used)) = "GPL";
int _version __attribute__((section("version"), used)) = 0x40100;
$ cd ~/bin && ./hello-ebpf
$ perf record --event /tmp/test.o sleep 1
Using perf wrapper that supports hot-text. Try perf.real if you
encounter any issues.
Probe point 'does_not_exist' not found.
event syntax error: '/tmp/test.o'
                     \___ You need to check probing points in BPF file

(add -v to see detail)
Run 'perf list' for a list of valid events

 Usage: perf record [<options>] [<command>]
    or: perf record [<options>] -- <command> [<options>]

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

---------------------------------------------------

Next I changed the attribute to something that exists in the kernel.
As expected, it errors out
with permission problem:
$ cat /tmp/test.c
__attribute__((section("fork=fork_init"), used)) int fork(void *ctx) {
    return 0;
}
char _license[] __attribute__((section("license"), used)) = "GPL";
int _version __attribute__((section("version"), used)) = 0x40100;
$ grep fork_init /proc/kallsyms
ffffffff8146e250 T xfs_ifork_init_cow
ffffffff83980481 T fork_init
$ cd ~/bin && ./hello-ebpf
$ perf record --event /tmp/test.o sleep 1
Using perf wrapper that supports hot-text. Try perf.real if you
encounter any issues.
Failed to open kprobe_events: Permission denied
event syntax error: '/tmp/test.o'
                     \___ You need to be root

(add -v to see detail)
Run 'perf list' for a list of valid events

 Usage: perf record [<options>] [<command>]
    or: perf record [<options>] -- <command> [<options>]

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

---------------------------------------------------

So I reran as root, but this time I get an invalid syntax error:

# perf record --event /tmp/test.o -v sleep 1
Using perf wrapper that supports hot-text. Try perf.real if you
encounter any issues.
Failed to write event: Invalid argument
event syntax error: '/tmp/test.o'
                     \___ Invalid argument

(add -v to see detail)
Run 'perf list' for a list of valid events

 Usage: perf record [<options>] [<command>]
    or: perf record [<options>] -- <command> [<options>]

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

Is there a different way to attach a custom event probe point?

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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2021-12-29 19:01           ` Christy Lee
@ 2022-01-04 14:40             ` Jiri Olsa
  2022-01-05 13:49               ` Jiri Olsa
  0 siblings, 1 reply; 18+ messages in thread
From: Jiri Olsa @ 2022-01-04 14:40 UTC (permalink / raw)
  To: Christy Lee
  Cc: Andrii Nakryiko, Christy Lee, Andrii Nakryiko,
	Arnaldo Carvalho de Melo, bpf, linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

On Wed, Dec 29, 2021 at 11:01:35AM -0800, Christy Lee wrote:

SNIP

> > >
> > > I don't use it, I just know it's there.. that's why I asked ;-)
> > >
> > > it's possible to specify bpf program on the perf command line
> > > to be attached to event, like:
> > >
> > >       # cat tools/perf/examples/bpf/hello.c
> > >       #include <stdio.h>
> > >
> > >       int syscall_enter(openat)(void *args)
> > >       {
> > >               puts("Hello, world\n");
> > >               return 0;
> > >       }
> > >
> > >       license(GPL);
> > >       #
> > >       # perf trace -e openat,tools/perf/examples/bpf/hello.c cat /etc/passwd > /dev/null
> > >          0.016 (         ): __bpf_stdout__:Hello, world
> > >          0.018 ( 0.010 ms): cat/9079 openat(dfd: CWD, filename: /etc/ld.so.cache, flags: CLOEXEC) = 3
> > >          0.057 (         ): __bpf_stdout__:Hello, world
> > >          0.059 ( 0.011 ms): cat/9079 openat(dfd: CWD, filename: /lib64/libc.so.6, flags: CLOEXEC) = 3
> > >          0.417 (         ): __bpf_stdout__:Hello, world
> > >          0.419 ( 0.009 ms): cat/9079 openat(dfd: CWD, filename: /etc/passwd) = 3
> > >       #
> > >
> > > I took that example from commit message
> [...]
> 
> I found the original commit aa3abf30bb28addcf593578d37447d42e3f65fc3
> that included a test case, but I'm having trouble reproducing it due to syntax
> error. I am running this on bpf-next master without my patches.
> 
> I ran 'perf test -v LLVM' and used it's output to generate a script for
> compiling the perf test object:
> 
> --------------------------------------------------
> $ cat ~/bin/hello-ebpf
> INPUT_FILE=/tmp/test.c
> OUTPUT_FILE=/tmp/test.o
> 
> export KBUILD_DIR=/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build
> export NR_CPUS=56
> export LINUX_VERSION_CODE=0x50c00
> export CLANG_EXEC=/data/users/christylee/devtools/llvm/latest/bin/clang
> export CLANG_OPTIONS=-xc
> export KERNEL_INC_OPTIONS="-nostdinc -isystem
> /data/users/christylee/devtools/gcc/10.3.0/lib/gcc/x86_64-pc-linux-gnu/10.3.0/include
> -I./arch/\
> x86/include -I./arch/x86/include/generated  -I./include
> -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi
> -I./include/uapi -I./in\
> clude/generated/uapi -include ./include/linux/compiler-version.h
> -include ./include/linux/kconfig.h"
> export PERF_BPF_INC_OPTIONS=-I/home/christylee/lib/perf/include/bpf
> export WORKING_DIR=/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build
> export CLANG_SOURCE=-
> 
> rm -f $OUTPUT_FILE
> cat $INPUT_FILE |
> /data/users/christylee/devtools/llvm/latest/bin/clang -D__KERNEL__
> -D__NR_CPUS__=56 -DLINUX_VERSION_CODE=0x50c00 -xc  -I/ho\
> me/christylee/lib/perf/include/bpf  -nostdinc -isystem
> /data/users/christylee/devtools/gcc/10.3.0/lib/gcc/x86_64-pc-linux-gnu/10.3.0/include
> \
> -I./arch/x86/include -I./arch/x86/include/generated  -I./include
> -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi
> -I./include/ua\
> pi -I./include/generated/uapi -include
> ./include/linux/compiler-version.h -include ./include/linux/kconfig.h
> -Wno-unused-value -Wno-pointer-\
> sign -working-directory
> /lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build -c - -target bpf
> -O2 -o $OUTPUT_FILE
> --------------------------------------------------
> 
> I then wrote and compiled a script that ask to get asks to put a probe
> at a function that
> does not exists in the kernel, it errors out as expected:
> 
> $ cat /tmp/test.c
> __attribute__((section("fork=does_not_exist"), used)) int fork(void *ctx) {
>     return 0;
> }
> 
> char _license[] __attribute__((section("license"), used)) = "GPL";
> int _version __attribute__((section("version"), used)) = 0x40100;
> $ cd ~/bin && ./hello-ebpf
> $ perf record --event /tmp/test.o sleep 1
> Using perf wrapper that supports hot-text. Try perf.real if you
> encounter any issues.
> Probe point 'does_not_exist' not found.
> event syntax error: '/tmp/test.o'
>                      \___ You need to check probing points in BPF file
> 
> (add -v to see detail)
> Run 'perf list' for a list of valid events
> 
>  Usage: perf record [<options>] [<command>]
>     or: perf record [<options>] -- <command> [<options>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list
> available events
> 
> ---------------------------------------------------
> 
> Next I changed the attribute to something that exists in the kernel.
> As expected, it errors out
> with permission problem:
> $ cat /tmp/test.c
> __attribute__((section("fork=fork_init"), used)) int fork(void *ctx) {
>     return 0;
> }
> char _license[] __attribute__((section("license"), used)) = "GPL";
> int _version __attribute__((section("version"), used)) = 0x40100;
> $ grep fork_init /proc/kallsyms
> ffffffff8146e250 T xfs_ifork_init_cow
> ffffffff83980481 T fork_init
> $ cd ~/bin && ./hello-ebpf
> $ perf record --event /tmp/test.o sleep 1
> Using perf wrapper that supports hot-text. Try perf.real if you
> encounter any issues.
> Failed to open kprobe_events: Permission denied
> event syntax error: '/tmp/test.o'
>                      \___ You need to be root
> 
> (add -v to see detail)
> Run 'perf list' for a list of valid events
> 
>  Usage: perf record [<options>] [<command>]
>     or: perf record [<options>] -- <command> [<options>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list
> available events
> 
> ---------------------------------------------------
> 
> So I reran as root, but this time I get an invalid syntax error:
> 
> # perf record --event /tmp/test.o -v sleep 1
> Using perf wrapper that supports hot-text. Try perf.real if you
> encounter any issues.
> Failed to write event: Invalid argument
> event syntax error: '/tmp/test.o'
>                      \___ Invalid argument
> 
> (add -v to see detail)
> Run 'perf list' for a list of valid events
> 
>  Usage: perf record [<options>] [<command>]
>     or: perf record [<options>] -- <command> [<options>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list
> available events
> ---------------------------------------------------
> 
> Is there a different way to attach a custom event probe point?
> 

nice, good question ;-)

looks like there are no volunteers from original authors,
I'll check on that

thanks,
jirka


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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2022-01-04 14:40             ` Jiri Olsa
@ 2022-01-05 13:49               ` Jiri Olsa
  2022-01-06 17:54                 ` Christy Lee
  2022-01-06 20:25                 ` Arnaldo Carvalho de Melo
  0 siblings, 2 replies; 18+ messages in thread
From: Jiri Olsa @ 2022-01-05 13:49 UTC (permalink / raw)
  To: Christy Lee
  Cc: Andrii Nakryiko, Christy Lee, Andrii Nakryiko,
	Arnaldo Carvalho de Melo, bpf, linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

On Tue, Jan 04, 2022 at 03:40:49PM +0100, Jiri Olsa wrote:
> On Wed, Dec 29, 2021 at 11:01:35AM -0800, Christy Lee wrote:
> 
> SNIP
> 
> > > >
> > > > I don't use it, I just know it's there.. that's why I asked ;-)
> > > >
> > > > it's possible to specify bpf program on the perf command line
> > > > to be attached to event, like:
> > > >
> > > >       # cat tools/perf/examples/bpf/hello.c
> > > >       #include <stdio.h>
> > > >
> > > >       int syscall_enter(openat)(void *args)
> > > >       {
> > > >               puts("Hello, world\n");
> > > >               return 0;
> > > >       }
> > > >
> > > >       license(GPL);
> > > >       #
> > > >       # perf trace -e openat,tools/perf/examples/bpf/hello.c cat /etc/passwd > /dev/null
> > > >          0.016 (         ): __bpf_stdout__:Hello, world
> > > >          0.018 ( 0.010 ms): cat/9079 openat(dfd: CWD, filename: /etc/ld.so.cache, flags: CLOEXEC) = 3
> > > >          0.057 (         ): __bpf_stdout__:Hello, world
> > > >          0.059 ( 0.011 ms): cat/9079 openat(dfd: CWD, filename: /lib64/libc.so.6, flags: CLOEXEC) = 3
> > > >          0.417 (         ): __bpf_stdout__:Hello, world
> > > >          0.419 ( 0.009 ms): cat/9079 openat(dfd: CWD, filename: /etc/passwd) = 3
> > > >       #
> > > >
> > > > I took that example from commit message
> > [...]
> > 
> > I found the original commit aa3abf30bb28addcf593578d37447d42e3f65fc3
> > that included a test case, but I'm having trouble reproducing it due to syntax
> > error. I am running this on bpf-next master without my patches.
> > 
> > I ran 'perf test -v LLVM' and used it's output to generate a script for
> > compiling the perf test object:
> > 
> > --------------------------------------------------
> > $ cat ~/bin/hello-ebpf
> > INPUT_FILE=/tmp/test.c
> > OUTPUT_FILE=/tmp/test.o
> > 
> > export KBUILD_DIR=/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build
> > export NR_CPUS=56
> > export LINUX_VERSION_CODE=0x50c00
> > export CLANG_EXEC=/data/users/christylee/devtools/llvm/latest/bin/clang
> > export CLANG_OPTIONS=-xc
> > export KERNEL_INC_OPTIONS="-nostdinc -isystem
> > /data/users/christylee/devtools/gcc/10.3.0/lib/gcc/x86_64-pc-linux-gnu/10.3.0/include
> > -I./arch/\
> > x86/include -I./arch/x86/include/generated  -I./include
> > -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi
> > -I./include/uapi -I./in\
> > clude/generated/uapi -include ./include/linux/compiler-version.h
> > -include ./include/linux/kconfig.h"
> > export PERF_BPF_INC_OPTIONS=-I/home/christylee/lib/perf/include/bpf
> > export WORKING_DIR=/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build
> > export CLANG_SOURCE=-
> > 
> > rm -f $OUTPUT_FILE
> > cat $INPUT_FILE |
> > /data/users/christylee/devtools/llvm/latest/bin/clang -D__KERNEL__
> > -D__NR_CPUS__=56 -DLINUX_VERSION_CODE=0x50c00 -xc  -I/ho\
> > me/christylee/lib/perf/include/bpf  -nostdinc -isystem
> > /data/users/christylee/devtools/gcc/10.3.0/lib/gcc/x86_64-pc-linux-gnu/10.3.0/include
> > \
> > -I./arch/x86/include -I./arch/x86/include/generated  -I./include
> > -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi
> > -I./include/ua\
> > pi -I./include/generated/uapi -include
> > ./include/linux/compiler-version.h -include ./include/linux/kconfig.h
> > -Wno-unused-value -Wno-pointer-\
> > sign -working-directory
> > /lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build -c - -target bpf
> > -O2 -o $OUTPUT_FILE
> > --------------------------------------------------
> > 
> > I then wrote and compiled a script that ask to get asks to put a probe
> > at a function that
> > does not exists in the kernel, it errors out as expected:
> > 
> > $ cat /tmp/test.c
> > __attribute__((section("fork=does_not_exist"), used)) int fork(void *ctx) {
> >     return 0;
> > }
> > 
> > char _license[] __attribute__((section("license"), used)) = "GPL";
> > int _version __attribute__((section("version"), used)) = 0x40100;
> > $ cd ~/bin && ./hello-ebpf
> > $ perf record --event /tmp/test.o sleep 1
> > Using perf wrapper that supports hot-text. Try perf.real if you
> > encounter any issues.
> > Probe point 'does_not_exist' not found.
> > event syntax error: '/tmp/test.o'
> >                      \___ You need to check probing points in BPF file
> > 
> > (add -v to see detail)
> > Run 'perf list' for a list of valid events
> > 
> >  Usage: perf record [<options>] [<command>]
> >     or: perf record [<options>] -- <command> [<options>]
> > 
> >     -e, --event <event>   event selector. use 'perf list' to list
> > available events
> > 
> > ---------------------------------------------------
> > 
> > Next I changed the attribute to something that exists in the kernel.
> > As expected, it errors out
> > with permission problem:
> > $ cat /tmp/test.c
> > __attribute__((section("fork=fork_init"), used)) int fork(void *ctx) {
> >     return 0;
> > }
> > char _license[] __attribute__((section("license"), used)) = "GPL";
> > int _version __attribute__((section("version"), used)) = 0x40100;
> > $ grep fork_init /proc/kallsyms
> > ffffffff8146e250 T xfs_ifork_init_cow
> > ffffffff83980481 T fork_init
> > $ cd ~/bin && ./hello-ebpf
> > $ perf record --event /tmp/test.o sleep 1
> > Using perf wrapper that supports hot-text. Try perf.real if you
> > encounter any issues.
> > Failed to open kprobe_events: Permission denied
> > event syntax error: '/tmp/test.o'
> >                      \___ You need to be root
> > 
> > (add -v to see detail)
> > Run 'perf list' for a list of valid events
> > 
> >  Usage: perf record [<options>] [<command>]
> >     or: perf record [<options>] -- <command> [<options>]
> > 
> >     -e, --event <event>   event selector. use 'perf list' to list
> > available events
> > 
> > ---------------------------------------------------
> > 
> > So I reran as root, but this time I get an invalid syntax error:
> > 
> > # perf record --event /tmp/test.o -v sleep 1
> > Using perf wrapper that supports hot-text. Try perf.real if you
> > encounter any issues.
> > Failed to write event: Invalid argument
> > event syntax error: '/tmp/test.o'
> >                      \___ Invalid argument
> > 
> > (add -v to see detail)
> > Run 'perf list' for a list of valid events
> > 
> >  Usage: perf record [<options>] [<command>]
> >     or: perf record [<options>] -- <command> [<options>]
> > 
> >     -e, --event <event>   event selector. use 'perf list' to list
> > available events
> > ---------------------------------------------------
> > 
> > Is there a different way to attach a custom event probe point?
> > 
> 
> nice, good question ;-)
> 
> looks like there are no volunteers from original authors,
> I'll check on that

there's small bug in perf trace that makes it to die early,
(fix below) but other than that it works.. I'll send full
patch later

you need to specify full path for bpf object, not like in the
example I pasted above.. I recall fixing that in code because
it clashed with pmu syntax

so on fedora 35 I can run following with the change below:

	# ./perf trace -e openat,/home/jolsa/linux-qemu/tools/perf/examples/bpf/hello.c  
	/home/jolsa/linux-qemu/tools/perf/examples/bpf/hello.c:5:2: warning: variable length array folded to constant array as an extension [-Wgnu-folding-constant]
		puts("Hello, world\n");
		^
	/home/jolsa/lib/perf/include/bpf/stdio.h:14:10: note: expanded from macro 'puts'
		   char __from[__len] = from; \
			^
	1 warning generated.
	     0.000 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
	     0.016 ( 0.031 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
	     0.070 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
	     0.074 ( 0.011 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
	     0.097 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
	     0.101 ( 0.010 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
	     0.123 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
	     0.127 ( 0.010 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
	     0.148 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
	     0.152 ( 0.010 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
	     0.219 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
	...


jirka


---
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 97121fb45842..df9fc00b4cd6 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -3936,6 +3936,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
 	bool draining = false;
 
 	trace->live = true;
+	signal(SIGCHLD, sig_handler);
 
 	if (!trace->raw_augmented_syscalls) {
 		if (trace->trace_syscalls && trace__add_syscall_newtp(trace))
@@ -4884,7 +4885,6 @@ int cmd_trace(int argc, const char **argv)
 
 	signal(SIGSEGV, sighandler_dump_stack);
 	signal(SIGFPE, sighandler_dump_stack);
-	signal(SIGCHLD, sig_handler);
 	signal(SIGINT, sig_handler);
 
 	trace.evlist = evlist__new();


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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2022-01-05 13:49               ` Jiri Olsa
@ 2022-01-06 17:54                 ` Christy Lee
  2022-01-06 22:41                   ` Jiri Olsa
  2022-01-13 15:14                   ` Jiri Olsa
  2022-01-06 20:25                 ` Arnaldo Carvalho de Melo
  1 sibling, 2 replies; 18+ messages in thread
From: Christy Lee @ 2022-01-06 17:54 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Andrii Nakryiko, Christy Lee, Andrii Nakryiko,
	Arnaldo Carvalho de Melo, bpf, linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

Thank you so much, I was able to reproduce the original tests after applying
the bug fix. I will submit a new patch set with the more detailed comments.

The only deprecated functions that need to be removed after this would be
bpf_program__set_prep() (how perf sets the bpf prologue) and
bpf_program__nth_fd() (how perf leverages multi instance bpf). They look a
little more involved and I'm not sure how to approach those. Jiri, would you
mind taking a look at those please?

Christy

On Wed, Jan 5, 2022 at 5:49 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Jan 04, 2022 at 03:40:49PM +0100, Jiri Olsa wrote:
> > On Wed, Dec 29, 2021 at 11:01:35AM -0800, Christy Lee wrote:
> >
> > SNIP
> >
> > > > >
> > > > > I don't use it, I just know it's there.. that's why I asked ;-)
> > > > >
> > > > > it's possible to specify bpf program on the perf command line
> > > > > to be attached to event, like:
> > > > >
> > > > >       # cat tools/perf/examples/bpf/hello.c
> > > > >       #include <stdio.h>
> > > > >
> > > > >       int syscall_enter(openat)(void *args)
> > > > >       {
> > > > >               puts("Hello, world\n");
> > > > >               return 0;
> > > > >       }
> > > > >
> > > > >       license(GPL);
> > > > >       #
> > > > >       # perf trace -e openat,tools/perf/examples/bpf/hello.c cat /etc/passwd > /dev/null
> > > > >          0.016 (         ): __bpf_stdout__:Hello, world
> > > > >          0.018 ( 0.010 ms): cat/9079 openat(dfd: CWD, filename: /etc/ld.so.cache, flags: CLOEXEC) = 3
> > > > >          0.057 (         ): __bpf_stdout__:Hello, world
> > > > >          0.059 ( 0.011 ms): cat/9079 openat(dfd: CWD, filename: /lib64/libc.so.6, flags: CLOEXEC) = 3
> > > > >          0.417 (         ): __bpf_stdout__:Hello, world
> > > > >          0.419 ( 0.009 ms): cat/9079 openat(dfd: CWD, filename: /etc/passwd) = 3
> > > > >       #
> > > > >
> > > > > I took that example from commit message
> > > [...]
> > >
> > > I found the original commit aa3abf30bb28addcf593578d37447d42e3f65fc3
> > > that included a test case, but I'm having trouble reproducing it due to syntax
> > > error. I am running this on bpf-next master without my patches.
> > >
> > > I ran 'perf test -v LLVM' and used it's output to generate a script for
> > > compiling the perf test object:
> > >
> > > --------------------------------------------------
> > > $ cat ~/bin/hello-ebpf
> > > INPUT_FILE=/tmp/test.c
> > > OUTPUT_FILE=/tmp/test.o
> > >
> > > export KBUILD_DIR=/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build
> > > export NR_CPUS=56
> > > export LINUX_VERSION_CODE=0x50c00
> > > export CLANG_EXEC=/data/users/christylee/devtools/llvm/latest/bin/clang
> > > export CLANG_OPTIONS=-xc
> > > export KERNEL_INC_OPTIONS="-nostdinc -isystem
> > > /data/users/christylee/devtools/gcc/10.3.0/lib/gcc/x86_64-pc-linux-gnu/10.3.0/include
> > > -I./arch/\
> > > x86/include -I./arch/x86/include/generated  -I./include
> > > -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi
> > > -I./include/uapi -I./in\
> > > clude/generated/uapi -include ./include/linux/compiler-version.h
> > > -include ./include/linux/kconfig.h"
> > > export PERF_BPF_INC_OPTIONS=-I/home/christylee/lib/perf/include/bpf
> > > export WORKING_DIR=/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build
> > > export CLANG_SOURCE=-
> > >
> > > rm -f $OUTPUT_FILE
> > > cat $INPUT_FILE |
> > > /data/users/christylee/devtools/llvm/latest/bin/clang -D__KERNEL__
> > > -D__NR_CPUS__=56 -DLINUX_VERSION_CODE=0x50c00 -xc  -I/ho\
> > > me/christylee/lib/perf/include/bpf  -nostdinc -isystem
> > > /data/users/christylee/devtools/gcc/10.3.0/lib/gcc/x86_64-pc-linux-gnu/10.3.0/include
> > > \
> > > -I./arch/x86/include -I./arch/x86/include/generated  -I./include
> > > -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi
> > > -I./include/ua\
> > > pi -I./include/generated/uapi -include
> > > ./include/linux/compiler-version.h -include ./include/linux/kconfig.h
> > > -Wno-unused-value -Wno-pointer-\
> > > sign -working-directory
> > > /lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build -c - -target bpf
> > > -O2 -o $OUTPUT_FILE
> > > --------------------------------------------------
> > >
> > > I then wrote and compiled a script that ask to get asks to put a probe
> > > at a function that
> > > does not exists in the kernel, it errors out as expected:
> > >
> > > $ cat /tmp/test.c
> > > __attribute__((section("fork=does_not_exist"), used)) int fork(void *ctx) {
> > >     return 0;
> > > }
> > >
> > > char _license[] __attribute__((section("license"), used)) = "GPL";
> > > int _version __attribute__((section("version"), used)) = 0x40100;
> > > $ cd ~/bin && ./hello-ebpf
> > > $ perf record --event /tmp/test.o sleep 1
> > > Using perf wrapper that supports hot-text. Try perf.real if you
> > > encounter any issues.
> > > Probe point 'does_not_exist' not found.
> > > event syntax error: '/tmp/test.o'
> > >                      \___ You need to check probing points in BPF file
> > >
> > > (add -v to see detail)
> > > Run 'perf list' for a list of valid events
> > >
> > >  Usage: perf record [<options>] [<command>]
> > >     or: perf record [<options>] -- <command> [<options>]
> > >
> > >     -e, --event <event>   event selector. use 'perf list' to list
> > > available events
> > >
> > > ---------------------------------------------------
> > >
> > > Next I changed the attribute to something that exists in the kernel.
> > > As expected, it errors out
> > > with permission problem:
> > > $ cat /tmp/test.c
> > > __attribute__((section("fork=fork_init"), used)) int fork(void *ctx) {
> > >     return 0;
> > > }
> > > char _license[] __attribute__((section("license"), used)) = "GPL";
> > > int _version __attribute__((section("version"), used)) = 0x40100;
> > > $ grep fork_init /proc/kallsyms
> > > ffffffff8146e250 T xfs_ifork_init_cow
> > > ffffffff83980481 T fork_init
> > > $ cd ~/bin && ./hello-ebpf
> > > $ perf record --event /tmp/test.o sleep 1
> > > Using perf wrapper that supports hot-text. Try perf.real if you
> > > encounter any issues.
> > > Failed to open kprobe_events: Permission denied
> > > event syntax error: '/tmp/test.o'
> > >                      \___ You need to be root
> > >
> > > (add -v to see detail)
> > > Run 'perf list' for a list of valid events
> > >
> > >  Usage: perf record [<options>] [<command>]
> > >     or: perf record [<options>] -- <command> [<options>]
> > >
> > >     -e, --event <event>   event selector. use 'perf list' to list
> > > available events
> > >
> > > ---------------------------------------------------
> > >
> > > So I reran as root, but this time I get an invalid syntax error:
> > >
> > > # perf record --event /tmp/test.o -v sleep 1
> > > Using perf wrapper that supports hot-text. Try perf.real if you
> > > encounter any issues.
> > > Failed to write event: Invalid argument
> > > event syntax error: '/tmp/test.o'
> > >                      \___ Invalid argument
> > >
> > > (add -v to see detail)
> > > Run 'perf list' for a list of valid events
> > >
> > >  Usage: perf record [<options>] [<command>]
> > >     or: perf record [<options>] -- <command> [<options>]
> > >
> > >     -e, --event <event>   event selector. use 'perf list' to list
> > > available events
> > > ---------------------------------------------------
> > >
> > > Is there a different way to attach a custom event probe point?
> > >
> >
> > nice, good question ;-)
> >
> > looks like there are no volunteers from original authors,
> > I'll check on that
>
> there's small bug in perf trace that makes it to die early,
> (fix below) but other than that it works.. I'll send full
> patch later
>
> you need to specify full path for bpf object, not like in the
> example I pasted above.. I recall fixing that in code because
> it clashed with pmu syntax
>
> so on fedora 35 I can run following with the change below:
>
>         # ./perf trace -e openat,/home/jolsa/linux-qemu/tools/perf/examples/bpf/hello.c
>         /home/jolsa/linux-qemu/tools/perf/examples/bpf/hello.c:5:2: warning: variable length array folded to constant array as an extension [-Wgnu-folding-constant]
>                 puts("Hello, world\n");
>                 ^
>         /home/jolsa/lib/perf/include/bpf/stdio.h:14:10: note: expanded from macro 'puts'
>                    char __from[__len] = from; \
>                         ^
>         1 warning generated.
>              0.000 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
>              0.016 ( 0.031 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
>              0.070 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
>              0.074 ( 0.011 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
>              0.097 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
>              0.101 ( 0.010 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
>              0.123 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
>              0.127 ( 0.010 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
>              0.148 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
>              0.152 ( 0.010 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
>              0.219 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
>         ...
>
>
> jirka
>
>
> ---
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 97121fb45842..df9fc00b4cd6 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -3936,6 +3936,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>         bool draining = false;
>
>         trace->live = true;
> +       signal(SIGCHLD, sig_handler);
>
>         if (!trace->raw_augmented_syscalls) {
>                 if (trace->trace_syscalls && trace__add_syscall_newtp(trace))
> @@ -4884,7 +4885,6 @@ int cmd_trace(int argc, const char **argv)
>
>         signal(SIGSEGV, sighandler_dump_stack);
>         signal(SIGFPE, sighandler_dump_stack);
> -       signal(SIGCHLD, sig_handler);
>         signal(SIGINT, sig_handler);
>
>         trace.evlist = evlist__new();
>

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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2022-01-05 13:49               ` Jiri Olsa
  2022-01-06 17:54                 ` Christy Lee
@ 2022-01-06 20:25                 ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 18+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-01-06 20:25 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Christy Lee, Andrii Nakryiko, Christy Lee, Andrii Nakryiko, bpf,
	linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

Em Wed, Jan 05, 2022 at 02:49:03PM +0100, Jiri Olsa escreveu:
> On Tue, Jan 04, 2022 at 03:40:49PM +0100, Jiri Olsa wrote:
> > On Wed, Dec 29, 2021 at 11:01:35AM -0800, Christy Lee wrote:
> > 
> > SNIP
> > 
> > > > >
> > > > > I don't use it, I just know it's there.. that's why I asked ;-)
> > > > >
> > > > > it's possible to specify bpf program on the perf command line
> > > > > to be attached to event, like:
> > > > >
> > > > >       # cat tools/perf/examples/bpf/hello.c
> > > > >       #include <stdio.h>
> > > > >
> > > > >       int syscall_enter(openat)(void *args)
> > > > >       {
> > > > >               puts("Hello, world\n");
> > > > >               return 0;
> > > > >       }
> > > > >
> > > > >       license(GPL);
> > > > >       #
> > > > >       # perf trace -e openat,tools/perf/examples/bpf/hello.c cat /etc/passwd > /dev/null
> > > > >          0.016 (         ): __bpf_stdout__:Hello, world
> > > > >          0.018 ( 0.010 ms): cat/9079 openat(dfd: CWD, filename: /etc/ld.so.cache, flags: CLOEXEC) = 3
> > > > >          0.057 (         ): __bpf_stdout__:Hello, world
> > > > >          0.059 ( 0.011 ms): cat/9079 openat(dfd: CWD, filename: /lib64/libc.so.6, flags: CLOEXEC) = 3
> > > > >          0.417 (         ): __bpf_stdout__:Hello, world
> > > > >          0.419 ( 0.009 ms): cat/9079 openat(dfd: CWD, filename: /etc/passwd) = 3
> > > > >       #
> > > > >
> > > > > I took that example from commit message
> > > [...]
> > > 
> > > I found the original commit aa3abf30bb28addcf593578d37447d42e3f65fc3
> > > that included a test case, but I'm having trouble reproducing it due to syntax
> > > error. I am running this on bpf-next master without my patches.
> > > 
> > > I ran 'perf test -v LLVM' and used it's output to generate a script for
> > > compiling the perf test object:
> > > 
> > > --------------------------------------------------
> > > $ cat ~/bin/hello-ebpf
> > > INPUT_FILE=/tmp/test.c
> > > OUTPUT_FILE=/tmp/test.o
> > > 
> > > export KBUILD_DIR=/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build
> > > export NR_CPUS=56
> > > export LINUX_VERSION_CODE=0x50c00
> > > export CLANG_EXEC=/data/users/christylee/devtools/llvm/latest/bin/clang
> > > export CLANG_OPTIONS=-xc
> > > export KERNEL_INC_OPTIONS="-nostdinc -isystem
> > > /data/users/christylee/devtools/gcc/10.3.0/lib/gcc/x86_64-pc-linux-gnu/10.3.0/include
> > > -I./arch/\
> > > x86/include -I./arch/x86/include/generated  -I./include
> > > -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi
> > > -I./include/uapi -I./in\
> > > clude/generated/uapi -include ./include/linux/compiler-version.h
> > > -include ./include/linux/kconfig.h"
> > > export PERF_BPF_INC_OPTIONS=-I/home/christylee/lib/perf/include/bpf
> > > export WORKING_DIR=/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build
> > > export CLANG_SOURCE=-
> > > 
> > > rm -f $OUTPUT_FILE
> > > cat $INPUT_FILE |
> > > /data/users/christylee/devtools/llvm/latest/bin/clang -D__KERNEL__
> > > -D__NR_CPUS__=56 -DLINUX_VERSION_CODE=0x50c00 -xc  -I/ho\
> > > me/christylee/lib/perf/include/bpf  -nostdinc -isystem
> > > /data/users/christylee/devtools/gcc/10.3.0/lib/gcc/x86_64-pc-linux-gnu/10.3.0/include
> > > \
> > > -I./arch/x86/include -I./arch/x86/include/generated  -I./include
> > > -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi
> > > -I./include/ua\
> > > pi -I./include/generated/uapi -include
> > > ./include/linux/compiler-version.h -include ./include/linux/kconfig.h
> > > -Wno-unused-value -Wno-pointer-\
> > > sign -working-directory
> > > /lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build -c - -target bpf
> > > -O2 -o $OUTPUT_FILE
> > > --------------------------------------------------
> > > 
> > > I then wrote and compiled a script that ask to get asks to put a probe
> > > at a function that
> > > does not exists in the kernel, it errors out as expected:
> > > 
> > > $ cat /tmp/test.c
> > > __attribute__((section("fork=does_not_exist"), used)) int fork(void *ctx) {
> > >     return 0;
> > > }
> > > 
> > > char _license[] __attribute__((section("license"), used)) = "GPL";
> > > int _version __attribute__((section("version"), used)) = 0x40100;
> > > $ cd ~/bin && ./hello-ebpf
> > > $ perf record --event /tmp/test.o sleep 1
> > > Using perf wrapper that supports hot-text. Try perf.real if you
> > > encounter any issues.
> > > Probe point 'does_not_exist' not found.
> > > event syntax error: '/tmp/test.o'
> > >                      \___ You need to check probing points in BPF file
> > > 
> > > (add -v to see detail)
> > > Run 'perf list' for a list of valid events
> > > 
> > >  Usage: perf record [<options>] [<command>]
> > >     or: perf record [<options>] -- <command> [<options>]
> > > 
> > >     -e, --event <event>   event selector. use 'perf list' to list
> > > available events
> > > 
> > > ---------------------------------------------------
> > > 
> > > Next I changed the attribute to something that exists in the kernel.
> > > As expected, it errors out
> > > with permission problem:
> > > $ cat /tmp/test.c
> > > __attribute__((section("fork=fork_init"), used)) int fork(void *ctx) {
> > >     return 0;
> > > }
> > > char _license[] __attribute__((section("license"), used)) = "GPL";
> > > int _version __attribute__((section("version"), used)) = 0x40100;
> > > $ grep fork_init /proc/kallsyms
> > > ffffffff8146e250 T xfs_ifork_init_cow
> > > ffffffff83980481 T fork_init
> > > $ cd ~/bin && ./hello-ebpf
> > > $ perf record --event /tmp/test.o sleep 1
> > > Using perf wrapper that supports hot-text. Try perf.real if you
> > > encounter any issues.
> > > Failed to open kprobe_events: Permission denied
> > > event syntax error: '/tmp/test.o'
> > >                      \___ You need to be root
> > > 
> > > (add -v to see detail)
> > > Run 'perf list' for a list of valid events
> > > 
> > >  Usage: perf record [<options>] [<command>]
> > >     or: perf record [<options>] -- <command> [<options>]
> > > 
> > >     -e, --event <event>   event selector. use 'perf list' to list
> > > available events
> > > 
> > > ---------------------------------------------------
> > > 
> > > So I reran as root, but this time I get an invalid syntax error:
> > > 
> > > # perf record --event /tmp/test.o -v sleep 1
> > > Using perf wrapper that supports hot-text. Try perf.real if you
> > > encounter any issues.
> > > Failed to write event: Invalid argument
> > > event syntax error: '/tmp/test.o'
> > >                      \___ Invalid argument
> > > 
> > > (add -v to see detail)
> > > Run 'perf list' for a list of valid events
> > > 
> > >  Usage: perf record [<options>] [<command>]
> > >     or: perf record [<options>] -- <command> [<options>]
> > > 
> > >     -e, --event <event>   event selector. use 'perf list' to list
> > > available events
> > > ---------------------------------------------------
> > > 
> > > Is there a different way to attach a custom event probe point?
> > > 
> > 
> > nice, good question ;-)
> > 
> > looks like there are no volunteers from original authors,
> > I'll check on that
> 
> there's small bug in perf trace that makes it to die early,
> (fix below) but other than that it works.. I'll send full
> patch later

Ok, waiting then.

- Arnaldo
 
> you need to specify full path for bpf object, not like in the
> example I pasted above.. I recall fixing that in code because
> it clashed with pmu syntax
> 
> so on fedora 35 I can run following with the change below:
> 
> 	# ./perf trace -e openat,/home/jolsa/linux-qemu/tools/perf/examples/bpf/hello.c  
> 	/home/jolsa/linux-qemu/tools/perf/examples/bpf/hello.c:5:2: warning: variable length array folded to constant array as an extension [-Wgnu-folding-constant]
> 		puts("Hello, world\n");
> 		^
> 	/home/jolsa/lib/perf/include/bpf/stdio.h:14:10: note: expanded from macro 'puts'
> 		   char __from[__len] = from; \
> 			^
> 	1 warning generated.
> 	     0.000 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
> 	     0.016 ( 0.031 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
> 	     0.070 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
> 	     0.074 ( 0.011 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
> 	     0.097 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
> 	     0.101 ( 0.010 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
> 	     0.123 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
> 	     0.127 ( 0.010 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
> 	     0.148 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
> 	     0.152 ( 0.010 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
> 	     0.219 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
> 	...
> 
> 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 97121fb45842..df9fc00b4cd6 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -3936,6 +3936,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>  	bool draining = false;
>  
>  	trace->live = true;
> +	signal(SIGCHLD, sig_handler);
>  
>  	if (!trace->raw_augmented_syscalls) {
>  		if (trace->trace_syscalls && trace__add_syscall_newtp(trace))
> @@ -4884,7 +4885,6 @@ int cmd_trace(int argc, const char **argv)
>  
>  	signal(SIGSEGV, sighandler_dump_stack);
>  	signal(SIGFPE, sighandler_dump_stack);
> -	signal(SIGCHLD, sig_handler);
>  	signal(SIGINT, sig_handler);
>  
>  	trace.evlist = evlist__new();

-- 

- Arnaldo

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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2022-01-06 17:54                 ` Christy Lee
@ 2022-01-06 22:41                   ` Jiri Olsa
  2022-01-13 15:14                   ` Jiri Olsa
  1 sibling, 0 replies; 18+ messages in thread
From: Jiri Olsa @ 2022-01-06 22:41 UTC (permalink / raw)
  To: Christy Lee
  Cc: Andrii Nakryiko, Christy Lee, Andrii Nakryiko,
	Arnaldo Carvalho de Melo, bpf, linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

On Thu, Jan 06, 2022 at 09:54:38AM -0800, Christy Lee wrote:
> Thank you so much, I was able to reproduce the original tests after applying
> the bug fix. I will submit a new patch set with the more detailed comments.

great

> 
> The only deprecated functions that need to be removed after this would be
> bpf_program__set_prep() (how perf sets the bpf prologue) and
> bpf_program__nth_fd() (how perf leverages multi instance bpf). They look a
> little more involved and I'm not sure how to approach those. Jiri, would you
> mind taking a look at those please?

yep, I plan to check on that

thanks,
jirka

> 
> Christy
> 
> On Wed, Jan 5, 2022 at 5:49 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Tue, Jan 04, 2022 at 03:40:49PM +0100, Jiri Olsa wrote:
> > > On Wed, Dec 29, 2021 at 11:01:35AM -0800, Christy Lee wrote:
> > >
> > > SNIP
> > >
> > > > > >
> > > > > > I don't use it, I just know it's there.. that's why I asked ;-)
> > > > > >
> > > > > > it's possible to specify bpf program on the perf command line
> > > > > > to be attached to event, like:
> > > > > >
> > > > > >       # cat tools/perf/examples/bpf/hello.c
> > > > > >       #include <stdio.h>
> > > > > >
> > > > > >       int syscall_enter(openat)(void *args)
> > > > > >       {
> > > > > >               puts("Hello, world\n");
> > > > > >               return 0;
> > > > > >       }
> > > > > >
> > > > > >       license(GPL);
> > > > > >       #
> > > > > >       # perf trace -e openat,tools/perf/examples/bpf/hello.c cat /etc/passwd > /dev/null
> > > > > >          0.016 (         ): __bpf_stdout__:Hello, world
> > > > > >          0.018 ( 0.010 ms): cat/9079 openat(dfd: CWD, filename: /etc/ld.so.cache, flags: CLOEXEC) = 3
> > > > > >          0.057 (         ): __bpf_stdout__:Hello, world
> > > > > >          0.059 ( 0.011 ms): cat/9079 openat(dfd: CWD, filename: /lib64/libc.so.6, flags: CLOEXEC) = 3
> > > > > >          0.417 (         ): __bpf_stdout__:Hello, world
> > > > > >          0.419 ( 0.009 ms): cat/9079 openat(dfd: CWD, filename: /etc/passwd) = 3
> > > > > >       #
> > > > > >
> > > > > > I took that example from commit message
> > > > [...]
> > > >
> > > > I found the original commit aa3abf30bb28addcf593578d37447d42e3f65fc3
> > > > that included a test case, but I'm having trouble reproducing it due to syntax
> > > > error. I am running this on bpf-next master without my patches.
> > > >
> > > > I ran 'perf test -v LLVM' and used it's output to generate a script for
> > > > compiling the perf test object:
> > > >
> > > > --------------------------------------------------
> > > > $ cat ~/bin/hello-ebpf
> > > > INPUT_FILE=/tmp/test.c
> > > > OUTPUT_FILE=/tmp/test.o
> > > >
> > > > export KBUILD_DIR=/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build
> > > > export NR_CPUS=56
> > > > export LINUX_VERSION_CODE=0x50c00
> > > > export CLANG_EXEC=/data/users/christylee/devtools/llvm/latest/bin/clang
> > > > export CLANG_OPTIONS=-xc
> > > > export KERNEL_INC_OPTIONS="-nostdinc -isystem
> > > > /data/users/christylee/devtools/gcc/10.3.0/lib/gcc/x86_64-pc-linux-gnu/10.3.0/include
> > > > -I./arch/\
> > > > x86/include -I./arch/x86/include/generated  -I./include
> > > > -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi
> > > > -I./include/uapi -I./in\
> > > > clude/generated/uapi -include ./include/linux/compiler-version.h
> > > > -include ./include/linux/kconfig.h"
> > > > export PERF_BPF_INC_OPTIONS=-I/home/christylee/lib/perf/include/bpf
> > > > export WORKING_DIR=/lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build
> > > > export CLANG_SOURCE=-
> > > >
> > > > rm -f $OUTPUT_FILE
> > > > cat $INPUT_FILE |
> > > > /data/users/christylee/devtools/llvm/latest/bin/clang -D__KERNEL__
> > > > -D__NR_CPUS__=56 -DLINUX_VERSION_CODE=0x50c00 -xc  -I/ho\
> > > > me/christylee/lib/perf/include/bpf  -nostdinc -isystem
> > > > /data/users/christylee/devtools/gcc/10.3.0/lib/gcc/x86_64-pc-linux-gnu/10.3.0/include
> > > > \
> > > > -I./arch/x86/include -I./arch/x86/include/generated  -I./include
> > > > -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi
> > > > -I./include/ua\
> > > > pi -I./include/generated/uapi -include
> > > > ./include/linux/compiler-version.h -include ./include/linux/kconfig.h
> > > > -Wno-unused-value -Wno-pointer-\
> > > > sign -working-directory
> > > > /lib/modules/5.12.0-0_fbk2_3390_g7ecb4ac46d7f/build -c - -target bpf
> > > > -O2 -o $OUTPUT_FILE
> > > > --------------------------------------------------
> > > >
> > > > I then wrote and compiled a script that ask to get asks to put a probe
> > > > at a function that
> > > > does not exists in the kernel, it errors out as expected:
> > > >
> > > > $ cat /tmp/test.c
> > > > __attribute__((section("fork=does_not_exist"), used)) int fork(void *ctx) {
> > > >     return 0;
> > > > }
> > > >
> > > > char _license[] __attribute__((section("license"), used)) = "GPL";
> > > > int _version __attribute__((section("version"), used)) = 0x40100;
> > > > $ cd ~/bin && ./hello-ebpf
> > > > $ perf record --event /tmp/test.o sleep 1
> > > > Using perf wrapper that supports hot-text. Try perf.real if you
> > > > encounter any issues.
> > > > Probe point 'does_not_exist' not found.
> > > > event syntax error: '/tmp/test.o'
> > > >                      \___ You need to check probing points in BPF file
> > > >
> > > > (add -v to see detail)
> > > > Run 'perf list' for a list of valid events
> > > >
> > > >  Usage: perf record [<options>] [<command>]
> > > >     or: perf record [<options>] -- <command> [<options>]
> > > >
> > > >     -e, --event <event>   event selector. use 'perf list' to list
> > > > available events
> > > >
> > > > ---------------------------------------------------
> > > >
> > > > Next I changed the attribute to something that exists in the kernel.
> > > > As expected, it errors out
> > > > with permission problem:
> > > > $ cat /tmp/test.c
> > > > __attribute__((section("fork=fork_init"), used)) int fork(void *ctx) {
> > > >     return 0;
> > > > }
> > > > char _license[] __attribute__((section("license"), used)) = "GPL";
> > > > int _version __attribute__((section("version"), used)) = 0x40100;
> > > > $ grep fork_init /proc/kallsyms
> > > > ffffffff8146e250 T xfs_ifork_init_cow
> > > > ffffffff83980481 T fork_init
> > > > $ cd ~/bin && ./hello-ebpf
> > > > $ perf record --event /tmp/test.o sleep 1
> > > > Using perf wrapper that supports hot-text. Try perf.real if you
> > > > encounter any issues.
> > > > Failed to open kprobe_events: Permission denied
> > > > event syntax error: '/tmp/test.o'
> > > >                      \___ You need to be root
> > > >
> > > > (add -v to see detail)
> > > > Run 'perf list' for a list of valid events
> > > >
> > > >  Usage: perf record [<options>] [<command>]
> > > >     or: perf record [<options>] -- <command> [<options>]
> > > >
> > > >     -e, --event <event>   event selector. use 'perf list' to list
> > > > available events
> > > >
> > > > ---------------------------------------------------
> > > >
> > > > So I reran as root, but this time I get an invalid syntax error:
> > > >
> > > > # perf record --event /tmp/test.o -v sleep 1
> > > > Using perf wrapper that supports hot-text. Try perf.real if you
> > > > encounter any issues.
> > > > Failed to write event: Invalid argument
> > > > event syntax error: '/tmp/test.o'
> > > >                      \___ Invalid argument
> > > >
> > > > (add -v to see detail)
> > > > Run 'perf list' for a list of valid events
> > > >
> > > >  Usage: perf record [<options>] [<command>]
> > > >     or: perf record [<options>] -- <command> [<options>]
> > > >
> > > >     -e, --event <event>   event selector. use 'perf list' to list
> > > > available events
> > > > ---------------------------------------------------
> > > >
> > > > Is there a different way to attach a custom event probe point?
> > > >
> > >
> > > nice, good question ;-)
> > >
> > > looks like there are no volunteers from original authors,
> > > I'll check on that
> >
> > there's small bug in perf trace that makes it to die early,
> > (fix below) but other than that it works.. I'll send full
> > patch later
> >
> > you need to specify full path for bpf object, not like in the
> > example I pasted above.. I recall fixing that in code because
> > it clashed with pmu syntax
> >
> > so on fedora 35 I can run following with the change below:
> >
> >         # ./perf trace -e openat,/home/jolsa/linux-qemu/tools/perf/examples/bpf/hello.c
> >         /home/jolsa/linux-qemu/tools/perf/examples/bpf/hello.c:5:2: warning: variable length array folded to constant array as an extension [-Wgnu-folding-constant]
> >                 puts("Hello, world\n");
> >                 ^
> >         /home/jolsa/lib/perf/include/bpf/stdio.h:14:10: note: expanded from macro 'puts'
> >                    char __from[__len] = from; \
> >                         ^
> >         1 warning generated.
> >              0.000 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
> >              0.016 ( 0.031 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
> >              0.070 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
> >              0.074 ( 0.011 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
> >              0.097 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
> >              0.101 ( 0.010 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
> >              0.123 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
> >              0.127 ( 0.010 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
> >              0.148 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
> >              0.152 ( 0.010 ms): systemd-resolv/1142 openat(dfd: CWD, filename: 0x6b1c4a70, flags: RDONLY|CLOEXEC)         = -1 ENOENT (No such file or directory)
> >              0.219 (         ): systemd-resolv/1142 __bpf_stdout__(Hello, world)
> >         ...
> >
> >
> > jirka
> >
> >
> > ---
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index 97121fb45842..df9fc00b4cd6 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -3936,6 +3936,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
> >         bool draining = false;
> >
> >         trace->live = true;
> > +       signal(SIGCHLD, sig_handler);
> >
> >         if (!trace->raw_augmented_syscalls) {
> >                 if (trace->trace_syscalls && trace__add_syscall_newtp(trace))
> > @@ -4884,7 +4885,6 @@ int cmd_trace(int argc, const char **argv)
> >
> >         signal(SIGSEGV, sighandler_dump_stack);
> >         signal(SIGFPE, sighandler_dump_stack);
> > -       signal(SIGCHLD, sig_handler);
> >         signal(SIGINT, sig_handler);
> >
> >         trace.evlist = evlist__new();
> >
> 


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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2022-01-06 17:54                 ` Christy Lee
  2022-01-06 22:41                   ` Jiri Olsa
@ 2022-01-13 15:14                   ` Jiri Olsa
  2022-01-14 21:00                     ` Andrii Nakryiko
  1 sibling, 1 reply; 18+ messages in thread
From: Jiri Olsa @ 2022-01-13 15:14 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, Christy Lee, Christy Lee,
	Arnaldo Carvalho de Melo, bpf, linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

On Thu, Jan 06, 2022 at 09:54:38AM -0800, Christy Lee wrote:
> Thank you so much, I was able to reproduce the original tests after applying
> the bug fix. I will submit a new patch set with the more detailed comments.
> 
> The only deprecated functions that need to be removed after this would be
> bpf_program__set_prep() (how perf sets the bpf prologue) and
> bpf_program__nth_fd() (how perf leverages multi instance bpf). They look a
> little more involved and I'm not sure how to approach those. Jiri, would you
> mind taking a look at those please?

hi,
I checked and here's the way perf uses this interface:

  - when bpf object/sources is specified on perf command line
    we use bpf_object__open to load it

  - user can define parameters in the section name for each bpf program
    like:

      SEC("lock_page=__lock_page page->flags")
      int lock_page(struct pt_regs *ctx, int err, unsigned long flags)
      {
             return 1;
      }

    which tells perf to 'prepare' some extra bpf code for the program,
    like to put value of 'page->flags' into 'flags' argument above

  - perf generates extra prologue code to retrieve this data and does
    that before the program is loaded by using bpf_program__set_prep
    callback

  - now the reason why we use bpf_program__set_prep for that, is because
    it allows to create multiple instances for one bpf program

  - we need multiple instances for single program, because probe can
    result in multiple attach addresses (like for inlined functions)
    with possible different ways of getting the arguments we need
    to load

I guess you want to get rid of that whole 'struct instances' related
stuff, is that right?

perf would need to load all the needed instances for program manually
and somehow bypass/workaround bpf_object__load.. is there a way to
manually add extra programs to bpf_object?

thoughts? ;-)

thanks,
jirka


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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2022-01-13 15:14                   ` Jiri Olsa
@ 2022-01-14 21:00                     ` Andrii Nakryiko
  2022-01-17  9:25                       ` Jiri Olsa
  0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2022-01-14 21:00 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Andrii Nakryiko, Christy Lee, Christy Lee,
	Arnaldo Carvalho de Melo, bpf, linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

On Thu, Jan 13, 2022 at 7:14 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Jan 06, 2022 at 09:54:38AM -0800, Christy Lee wrote:
> > Thank you so much, I was able to reproduce the original tests after applying
> > the bug fix. I will submit a new patch set with the more detailed comments.
> >
> > The only deprecated functions that need to be removed after this would be
> > bpf_program__set_prep() (how perf sets the bpf prologue) and
> > bpf_program__nth_fd() (how perf leverages multi instance bpf). They look a
> > little more involved and I'm not sure how to approach those. Jiri, would you
> > mind taking a look at those please?
>
> hi,
> I checked and here's the way perf uses this interface:
>
>   - when bpf object/sources is specified on perf command line
>     we use bpf_object__open to load it
>
>   - user can define parameters in the section name for each bpf program
>     like:
>
>       SEC("lock_page=__lock_page page->flags")
>       int lock_page(struct pt_regs *ctx, int err, unsigned long flags)
>       {
>              return 1;
>       }
>
>     which tells perf to 'prepare' some extra bpf code for the program,
>     like to put value of 'page->flags' into 'flags' argument above
>
>   - perf generates extra prologue code to retrieve this data and does
>     that before the program is loaded by using bpf_program__set_prep
>     callback
>
>   - now the reason why we use bpf_program__set_prep for that, is because
>     it allows to create multiple instances for one bpf program
>
>   - we need multiple instances for single program, because probe can
>     result in multiple attach addresses (like for inlined functions)
>     with possible different ways of getting the arguments we need
>     to load
>
> I guess you want to get rid of that whole 'struct instances' related
> stuff, is that right?
>
> perf would need to load all the needed instances for program manually
> and somehow bypass/workaround bpf_object__load.. is there a way to
> manually add extra programs to bpf_object?
>
> thoughts? ;-)

Sigh..

1. SEC("lock_page=__lock_page page->flags") will break in libbpf 1.0.
I'm going to add a way to provide a custom callback to handle such BPF
program sections by your custom code, but... Who's using this? Is
anyone using this? How is this used and for what? Would it be possible
to just kill this feature?

2. For creating multiple instances. I've added bpf_program__insns()
API to fetch exact bytecode as it is sent by libbpf to kernel. If you
fetch those instructions *after* the program is loaded, all the map
FDs and other stuff will be correct and resolved. At that point you
can use that to create as many copies as you want to with low-level
bpf_prog_load() API. You'll need to keep track of those FDs of clones,
but you'll have your multiple instances.

3. Do you really support attaching to inlined functions *and* also
fetching their input arguments? How does that even work, given that
the compiler can spread and reorder inlined function code as it sees
fit?...

But really, why does that feature exist at all? Why BPF program can't
fetch whatever it needs to fetch explicitly, like the rest of BPF
applications in the world do? Too much custom magic :( Especially with
the BPF_KPROBE() macro this is so trivial to do...

Can we kill this feature altogether, maybe? Pretty please? Such a
burden for everyone...

>
> thanks,
> jirka
>

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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2022-01-14 21:00                     ` Andrii Nakryiko
@ 2022-01-17  9:25                       ` Jiri Olsa
  2022-01-18 23:05                         ` Andrii Nakryiko
  2022-01-22 20:29                         ` Arnaldo Carvalho de Melo
  0 siblings, 2 replies; 18+ messages in thread
From: Jiri Olsa @ 2022-01-17  9:25 UTC (permalink / raw)
  To: Andrii Nakryiko, Arnaldo Carvalho de Melo
  Cc: Andrii Nakryiko, Christy Lee, Christy Lee, bpf, linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

On Fri, Jan 14, 2022 at 01:00:45PM -0800, Andrii Nakryiko wrote:
> On Thu, Jan 13, 2022 at 7:14 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Thu, Jan 06, 2022 at 09:54:38AM -0800, Christy Lee wrote:
> > > Thank you so much, I was able to reproduce the original tests after applying
> > > the bug fix. I will submit a new patch set with the more detailed comments.
> > >
> > > The only deprecated functions that need to be removed after this would be
> > > bpf_program__set_prep() (how perf sets the bpf prologue) and
> > > bpf_program__nth_fd() (how perf leverages multi instance bpf). They look a
> > > little more involved and I'm not sure how to approach those. Jiri, would you
> > > mind taking a look at those please?
> >
> > hi,
> > I checked and here's the way perf uses this interface:
> >
> >   - when bpf object/sources is specified on perf command line
> >     we use bpf_object__open to load it
> >
> >   - user can define parameters in the section name for each bpf program
> >     like:
> >
> >       SEC("lock_page=__lock_page page->flags")
> >       int lock_page(struct pt_regs *ctx, int err, unsigned long flags)
> >       {
> >              return 1;
> >       }
> >
> >     which tells perf to 'prepare' some extra bpf code for the program,
> >     like to put value of 'page->flags' into 'flags' argument above
> >
> >   - perf generates extra prologue code to retrieve this data and does
> >     that before the program is loaded by using bpf_program__set_prep
> >     callback
> >
> >   - now the reason why we use bpf_program__set_prep for that, is because
> >     it allows to create multiple instances for one bpf program
> >
> >   - we need multiple instances for single program, because probe can
> >     result in multiple attach addresses (like for inlined functions)
> >     with possible different ways of getting the arguments we need
> >     to load
> >
> > I guess you want to get rid of that whole 'struct instances' related
> > stuff, is that right?
> >
> > perf would need to load all the needed instances for program manually
> > and somehow bypass/workaround bpf_object__load.. is there a way to
> > manually add extra programs to bpf_object?
> >
> > thoughts? ;-)
> 
> Sigh..
> 
> 1. SEC("lock_page=__lock_page page->flags") will break in libbpf 1.0.
> I'm going to add a way to provide a custom callback to handle such BPF
> program sections by your custom code, but... Who's using this? Is
> anyone using this? How is this used and for what? Would it be possible
> to just kill this feature?

good question ;-) IMO it was added in the early ebpf times, when nobody
knew what will become the preferred way of doing things

we don't know if there are any users of this, but:

I had to go through the code to find out how to use it and it was broken
in perf trace for some time while nobody complained ;-) also I don't think
this is advertised anywhere in the doc

Arnaldo,
thoughts on removing this? ;-) I tried with the quick patch below, and
the standard perf trace ebpf support won't be affected by this

the patch is removing the support for generating the ebpf program prologue
which includes the usage of libbpf's instances APIs

we could also remove the special section config parsing, which is used
by prologue generation code

jirka


---
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index 96ad944ca6a8..d9ff537d999e 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -556,17 +556,6 @@ ifndef NO_LIBELF
       endif
     endif
 
-    ifndef NO_DWARF
-      ifdef PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET
-        CFLAGS += -DHAVE_BPF_PROLOGUE
-        $(call detected,CONFIG_BPF_PROLOGUE)
-      else
-        msg := $(warning BPF prologue is not supported by architecture $(SRCARCH), missing regs_query_register_offset());
-      endif
-    else
-      msg := $(warning DWARF support is off, BPF prologue is disabled);
-    endif
-
   endif # NO_LIBBPF
 endif # NO_LIBELF
 
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 6ac2160913ea..a04c02aed4c7 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -2685,20 +2685,6 @@ int cmd_record(int argc, const char **argv)
 	set_nobuild('\0', "clang-path", true);
 	set_nobuild('\0', "clang-opt", true);
 # undef set_nobuild
-#endif
-
-#ifndef HAVE_BPF_PROLOGUE
-# if !defined (HAVE_DWARF_SUPPORT)
-#  define REASON  "NO_DWARF=1"
-# elif !defined (HAVE_LIBBPF_SUPPORT)
-#  define REASON  "NO_LIBBPF=1"
-# else
-#  define REASON  "this architecture doesn't support BPF prologue"
-# endif
-# define set_nobuild(s, l, c) set_option_nobuild(record_options, s, l, REASON, c)
-	set_nobuild('\0', "vmlinux", true);
-# undef set_nobuild
-# undef REASON
 #endif
 
 	rec->opts.affinity = PERF_AFFINITY_SYS;
diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index 22662fc85cc9..0e3f24dfee2c 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -40,10 +40,6 @@ struct bpf_prog_priv {
 	char *sys_name;
 	char *evt_name;
 	struct perf_probe_event pev;
-	bool need_prologue;
-	struct bpf_insn *insns_buf;
-	int nr_types;
-	int *type_mapping;
 };
 
 static bool libbpf_initialized;
@@ -125,8 +121,6 @@ clear_prog_priv(struct bpf_program *prog __maybe_unused,
 	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);
@@ -409,220 +403,6 @@ static int bpf__prepare_probe(void)
 	return err;
 }
 
-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)
-{
-	struct bpf_prog_priv *priv = bpf_program__priv(prog);
-	struct probe_trace_event *tev;
-	struct perf_probe_event *pev;
-	struct bpf_insn *buf;
-	size_t prologue_cnt = 0;
-	int i, err;
-
-	if (IS_ERR_OR_NULL(priv) || priv->is_tp)
-		goto errout;
-
-	pev = &priv->pev;
-
-	if (n < 0 || n >= priv->nr_types)
-		goto errout;
-
-	/* Find a tev belongs to that type */
-	for (i = 0; i < pev->ntevs; i++) {
-		if (priv->type_mapping[i] == n)
-			break;
-	}
-
-	if (i >= pev->ntevs) {
-		pr_debug("Internal error: prologue type %d not found\n", n);
-		return -BPF_LOADER_ERRNO__PROLOGUE;
-	}
-
-	tev = &pev->tevs[i];
-
-	buf = priv->insns_buf;
-	err = bpf__gen_prologue(tev->args, tev->nargs,
-				buf, &prologue_cnt,
-				BPF_MAXINSNS - orig_insns_cnt);
-	if (err) {
-		const char *title;
-
-		title = bpf_program__section_name(prog);
-		pr_debug("Failed to generate prologue for program %s\n",
-			 title);
-		return err;
-	}
-
-	memcpy(&buf[prologue_cnt], orig_insns,
-	       sizeof(struct bpf_insn) * orig_insns_cnt);
-
-	res->new_insn_ptr = buf;
-	res->new_insn_cnt = prologue_cnt + orig_insns_cnt;
-	res->pfd = NULL;
-	return 0;
-
-errout:
-	pr_debug("Internal error in preproc_gen_prologue\n");
-	return -BPF_LOADER_ERRNO__PROLOGUE;
-}
-
-/*
- * compare_tev_args is reflexive, transitive and antisymmetric.
- * I can proof it but this margin is too narrow to contain.
- */
-static int compare_tev_args(const void *ptev1, const void *ptev2)
-{
-	int i, ret;
-	const struct probe_trace_event *tev1 =
-		*(const struct probe_trace_event **)ptev1;
-	const struct probe_trace_event *tev2 =
-		*(const struct probe_trace_event **)ptev2;
-
-	ret = tev2->nargs - tev1->nargs;
-	if (ret)
-		return ret;
-
-	for (i = 0; i < tev1->nargs; i++) {
-		struct probe_trace_arg *arg1, *arg2;
-		struct probe_trace_arg_ref *ref1, *ref2;
-
-		arg1 = &tev1->args[i];
-		arg2 = &tev2->args[i];
-
-		ret = strcmp(arg1->value, arg2->value);
-		if (ret)
-			return ret;
-
-		ref1 = arg1->ref;
-		ref2 = arg2->ref;
-
-		while (ref1 && ref2) {
-			ret = ref2->offset - ref1->offset;
-			if (ret)
-				return ret;
-
-			ref1 = ref1->next;
-			ref2 = ref2->next;
-		}
-
-		if (ref1 || ref2)
-			return ref2 ? 1 : -1;
-	}
-
-	return 0;
-}
-
-/*
- * Assign a type number to each tevs in a pev.
- * mapping is an array with same slots as tevs in that pev.
- * nr_types will be set to number of types.
- */
-static int map_prologue(struct perf_probe_event *pev, int *mapping,
-			int *nr_types)
-{
-	int i, type = 0;
-	struct probe_trace_event **ptevs;
-
-	size_t array_sz = sizeof(*ptevs) * pev->ntevs;
-
-	ptevs = malloc(array_sz);
-	if (!ptevs) {
-		pr_debug("Not enough memory: alloc ptevs failed\n");
-		return -ENOMEM;
-	}
-
-	pr_debug("In map_prologue, ntevs=%d\n", pev->ntevs);
-	for (i = 0; i < pev->ntevs; i++)
-		ptevs[i] = &pev->tevs[i];
-
-	qsort(ptevs, pev->ntevs, sizeof(*ptevs),
-	      compare_tev_args);
-
-	for (i = 0; i < pev->ntevs; i++) {
-		int n;
-
-		n = ptevs[i] - pev->tevs;
-		if (i == 0) {
-			mapping[n] = type;
-			pr_debug("mapping[%d]=%d\n", n, type);
-			continue;
-		}
-
-		if (compare_tev_args(ptevs + i, ptevs + i - 1) == 0)
-			mapping[n] = type;
-		else
-			mapping[n] = ++type;
-
-		pr_debug("mapping[%d]=%d\n", n, mapping[n]);
-	}
-	free(ptevs);
-	*nr_types = type + 1;
-
-	return 0;
-}
-
-static int hook_load_preprocessor(struct bpf_program *prog)
-{
-	struct bpf_prog_priv *priv = bpf_program__priv(prog);
-	struct perf_probe_event *pev;
-	bool need_prologue = false;
-	int err, i;
-
-	if (IS_ERR_OR_NULL(priv)) {
-		pr_debug("Internal error when hook preprocessor\n");
-		return -BPF_LOADER_ERRNO__INTERNAL;
-	}
-
-	if (priv->is_tp) {
-		priv->need_prologue = false;
-		return 0;
-	}
-
-	pev = &priv->pev;
-	for (i = 0; i < pev->ntevs; i++) {
-		struct probe_trace_event *tev = &pev->tevs[i];
-
-		if (tev->nargs > 0) {
-			need_prologue = true;
-			break;
-		}
-	}
-
-	/*
-	 * Since all tevs don't have argument, we don't need generate
-	 * prologue.
-	 */
-	if (!need_prologue) {
-		priv->need_prologue = false;
-		return 0;
-	}
-
-	priv->need_prologue = true;
-	priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
-	if (!priv->insns_buf) {
-		pr_debug("Not enough memory: alloc insns_buf failed\n");
-		return -ENOMEM;
-	}
-
-	priv->type_mapping = malloc(sizeof(int) * pev->ntevs);
-	if (!priv->type_mapping) {
-		pr_debug("Not enough memory: alloc type_mapping failed\n");
-		return -ENOMEM;
-	}
-	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;
-}
-
 int bpf__probe(struct bpf_object *obj)
 {
 	int err = 0;
@@ -669,18 +449,6 @@ int bpf__probe(struct bpf_object *obj)
 			pr_debug("bpf_probe: failed to apply perf probe events\n");
 			goto out;
 		}
-
-		/*
-		 * After probing, let's consider prologue, which
-		 * adds program fetcher to BPF programs.
-		 *
-		 * hook_load_preprocessor() hooks pre-processor
-		 * to bpf_program, let it generate prologue
-		 * dynamically during loading.
-		 */
-		err = hook_load_preprocessor(prog);
-		if (err)
-			goto out;
 	}
 out:
 	return err < 0 ? err : 0;
@@ -773,14 +541,7 @@ 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 {
-				fd = bpf_program__fd(prog);
-			}
-
+			fd = bpf_program__fd(prog);
 			if (fd < 0) {
 				pr_debug("bpf: failed to get file descriptor\n");
 				return fd;
diff --git a/tools/perf/util/bpf-prologue.c b/tools/perf/util/bpf-prologue.c
deleted file mode 100644
index 9887ae09242d..000000000000
--- a/tools/perf/util/bpf-prologue.c
+++ /dev/null
@@ -1,508 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * bpf-prologue.c
- *
- * Copyright (C) 2015 He Kuang <hekuang@huawei.com>
- * Copyright (C) 2015 Wang Nan <wangnan0@huawei.com>
- * Copyright (C) 2015 Huawei Inc.
- */
-
-#include <bpf/libbpf.h>
-#include "debug.h"
-#include "bpf-loader.h"
-#include "bpf-prologue.h"
-#include "probe-finder.h"
-#include <errno.h>
-#include <stdlib.h>
-#include <dwarf-regs.h>
-#include <linux/filter.h>
-
-#define BPF_REG_SIZE		8
-
-#define JMP_TO_ERROR_CODE	-1
-#define JMP_TO_SUCCESS_CODE	-2
-#define JMP_TO_USER_CODE	-3
-
-struct bpf_insn_pos {
-	struct bpf_insn *begin;
-	struct bpf_insn *end;
-	struct bpf_insn *pos;
-};
-
-static inline int
-pos_get_cnt(struct bpf_insn_pos *pos)
-{
-	return pos->pos - pos->begin;
-}
-
-static int
-append_insn(struct bpf_insn new_insn, struct bpf_insn_pos *pos)
-{
-	if (!pos->pos)
-		return -BPF_LOADER_ERRNO__PROLOGUE2BIG;
-
-	if (pos->pos + 1 >= pos->end) {
-		pr_err("bpf prologue: prologue too long\n");
-		pos->pos = NULL;
-		return -BPF_LOADER_ERRNO__PROLOGUE2BIG;
-	}
-
-	*(pos->pos)++ = new_insn;
-	return 0;
-}
-
-static int
-check_pos(struct bpf_insn_pos *pos)
-{
-	if (!pos->pos || pos->pos >= pos->end)
-		return -BPF_LOADER_ERRNO__PROLOGUE2BIG;
-	return 0;
-}
-
-/*
- * Convert type string (u8/u16/u32/u64/s8/s16/s32/s64 ..., see
- * Documentation/trace/kprobetrace.rst) to size field of BPF_LDX_MEM
- * instruction (BPF_{B,H,W,DW}).
- */
-static int
-argtype_to_ldx_size(const char *type)
-{
-	int arg_size = type ? atoi(&type[1]) : 64;
-
-	switch (arg_size) {
-	case 8:
-		return BPF_B;
-	case 16:
-		return BPF_H;
-	case 32:
-		return BPF_W;
-	case 64:
-	default:
-		return BPF_DW;
-	}
-}
-
-static const char *
-insn_sz_to_str(int insn_sz)
-{
-	switch (insn_sz) {
-	case BPF_B:
-		return "BPF_B";
-	case BPF_H:
-		return "BPF_H";
-	case BPF_W:
-		return "BPF_W";
-	case BPF_DW:
-		return "BPF_DW";
-	default:
-		return "UNKNOWN";
-	}
-}
-
-/* Give it a shorter name */
-#define ins(i, p) append_insn((i), (p))
-
-/*
- * Give a register name (in 'reg'), generate instruction to
- * load register into an eBPF register rd:
- *   'ldd target_reg, offset(ctx_reg)', where:
- * ctx_reg is pre initialized to pointer of 'struct pt_regs'.
- */
-static int
-gen_ldx_reg_from_ctx(struct bpf_insn_pos *pos, int ctx_reg,
-		     const char *reg, int target_reg)
-{
-	int offset = regs_query_register_offset(reg);
-
-	if (offset < 0) {
-		pr_err("bpf: prologue: failed to get register %s\n",
-		       reg);
-		return offset;
-	}
-	ins(BPF_LDX_MEM(BPF_DW, target_reg, ctx_reg, offset), pos);
-
-	return check_pos(pos);
-}
-
-/*
- * Generate a BPF_FUNC_probe_read function call.
- *
- * src_base_addr_reg is a register holding base address,
- * dst_addr_reg is a register holding dest address (on stack),
- * result is:
- *
- *  *[dst_addr_reg] = *([src_base_addr_reg] + offset)
- *
- * Arguments of BPF_FUNC_probe_read:
- *     ARG1: ptr to stack (dest)
- *     ARG2: size (8)
- *     ARG3: unsafe ptr (src)
- */
-static int
-gen_read_mem(struct bpf_insn_pos *pos,
-	     int src_base_addr_reg,
-	     int dst_addr_reg,
-	     long offset,
-	     int probeid)
-{
-	/* mov arg3, src_base_addr_reg */
-	if (src_base_addr_reg != BPF_REG_ARG3)
-		ins(BPF_MOV64_REG(BPF_REG_ARG3, src_base_addr_reg), pos);
-	/* add arg3, #offset */
-	if (offset)
-		ins(BPF_ALU64_IMM(BPF_ADD, BPF_REG_ARG3, offset), pos);
-
-	/* mov arg2, #reg_size */
-	ins(BPF_ALU64_IMM(BPF_MOV, BPF_REG_ARG2, BPF_REG_SIZE), pos);
-
-	/* mov arg1, dst_addr_reg */
-	if (dst_addr_reg != BPF_REG_ARG1)
-		ins(BPF_MOV64_REG(BPF_REG_ARG1, dst_addr_reg), pos);
-
-	/* Call probe_read  */
-	ins(BPF_EMIT_CALL(probeid), pos);
-	/*
-	 * Error processing: if read fail, goto error code,
-	 * will be relocated. Target should be the start of
-	 * error processing code.
-	 */
-	ins(BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, JMP_TO_ERROR_CODE),
-	    pos);
-
-	return check_pos(pos);
-}
-
-/*
- * Each arg should be bare register. Fetch and save them into argument
- * registers (r3 - r5).
- *
- * BPF_REG_1 should have been initialized with pointer to
- * 'struct pt_regs'.
- */
-static int
-gen_prologue_fastpath(struct bpf_insn_pos *pos,
-		      struct probe_trace_arg *args, int nargs)
-{
-	int i, err = 0;
-
-	for (i = 0; i < nargs; i++) {
-		err = gen_ldx_reg_from_ctx(pos, BPF_REG_1, args[i].value,
-					   BPF_PROLOGUE_START_ARG_REG + i);
-		if (err)
-			goto errout;
-	}
-
-	return check_pos(pos);
-errout:
-	return err;
-}
-
-/*
- * Slow path:
- *   At least one argument has the form of 'offset($rx)'.
- *
- * Following code first stores them into stack, then loads all of then
- * to r2 - r5.
- * Before final loading, the final result should be:
- *
- * low address
- * BPF_REG_FP - 24  ARG3
- * BPF_REG_FP - 16  ARG2
- * BPF_REG_FP - 8   ARG1
- * BPF_REG_FP
- * high address
- *
- * For each argument (described as: offn(...off2(off1(reg)))),
- * generates following code:
- *
- *  r7 <- fp
- *  r7 <- r7 - stack_offset  // Ideal code should initialize r7 using
- *                           // fp before generating args. However,
- *                           // eBPF won't regard r7 as stack pointer
- *                           // if it is generated by minus 8 from
- *                           // another stack pointer except fp.
- *                           // This is why we have to set r7
- *                           // to fp for each variable.
- *  r3 <- value of 'reg'-> generated using gen_ldx_reg_from_ctx()
- *  (r7) <- r3       // skip following instructions for bare reg
- *  r3 <- r3 + off1  . // skip if off1 == 0
- *  r2 <- 8           \
- *  r1 <- r7           |-> generated by gen_read_mem()
- *  call probe_read    /
- *  jnei r0, 0, err  ./
- *  r3 <- (r7)
- *  r3 <- r3 + off2  . // skip if off2 == 0
- *  r2 <- 8           \  // r2 may be broken by probe_read, so set again
- *  r1 <- r7           |-> generated by gen_read_mem()
- *  call probe_read    /
- *  jnei r0, 0, err  ./
- *  ...
- */
-static int
-gen_prologue_slowpath(struct bpf_insn_pos *pos,
-		      struct probe_trace_arg *args, int nargs)
-{
-	int err, i, probeid;
-
-	for (i = 0; i < nargs; i++) {
-		struct probe_trace_arg *arg = &args[i];
-		const char *reg = arg->value;
-		struct probe_trace_arg_ref *ref = NULL;
-		int stack_offset = (i + 1) * -8;
-
-		pr_debug("prologue: fetch arg %d, base reg is %s\n",
-			 i, reg);
-
-		/* value of base register is stored into ARG3 */
-		err = gen_ldx_reg_from_ctx(pos, BPF_REG_CTX, reg,
-					   BPF_REG_ARG3);
-		if (err) {
-			pr_err("prologue: failed to get offset of register %s\n",
-			       reg);
-			goto errout;
-		}
-
-		/* Make r7 the stack pointer. */
-		ins(BPF_MOV64_REG(BPF_REG_7, BPF_REG_FP), pos);
-		/* r7 += -8 */
-		ins(BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, stack_offset), pos);
-		/*
-		 * Store r3 (base register) onto stack
-		 * Ensure fp[offset] is set.
-		 * fp is the only valid base register when storing
-		 * into stack. We are not allowed to use r7 as base
-		 * register here.
-		 */
-		ins(BPF_STX_MEM(BPF_DW, BPF_REG_FP, BPF_REG_ARG3,
-				stack_offset), pos);
-
-		ref = arg->ref;
-		probeid = BPF_FUNC_probe_read_kernel;
-		while (ref) {
-			pr_debug("prologue: arg %d: offset %ld\n",
-				 i, ref->offset);
-
-			if (ref->user_access)
-				probeid = BPF_FUNC_probe_read_user;
-
-			err = gen_read_mem(pos, BPF_REG_3, BPF_REG_7,
-					   ref->offset, probeid);
-			if (err) {
-				pr_err("prologue: failed to generate probe_read function call\n");
-				goto errout;
-			}
-
-			ref = ref->next;
-			/*
-			 * Load previous result into ARG3. Use
-			 * BPF_REG_FP instead of r7 because verifier
-			 * allows FP based addressing only.
-			 */
-			if (ref)
-				ins(BPF_LDX_MEM(BPF_DW, BPF_REG_ARG3,
-						BPF_REG_FP, stack_offset), pos);
-		}
-	}
-
-	/* Final pass: read to registers */
-	for (i = 0; i < nargs; i++) {
-		int insn_sz = (args[i].ref) ? argtype_to_ldx_size(args[i].type) : BPF_DW;
-
-		pr_debug("prologue: load arg %d, insn_sz is %s\n",
-			 i, insn_sz_to_str(insn_sz));
-		ins(BPF_LDX_MEM(insn_sz, BPF_PROLOGUE_START_ARG_REG + i,
-				BPF_REG_FP, -BPF_REG_SIZE * (i + 1)), pos);
-	}
-
-	ins(BPF_JMP_IMM(BPF_JA, BPF_REG_0, 0, JMP_TO_SUCCESS_CODE), pos);
-
-	return check_pos(pos);
-errout:
-	return err;
-}
-
-static int
-prologue_relocate(struct bpf_insn_pos *pos, struct bpf_insn *error_code,
-		  struct bpf_insn *success_code, struct bpf_insn *user_code)
-{
-	struct bpf_insn *insn;
-
-	if (check_pos(pos))
-		return -BPF_LOADER_ERRNO__PROLOGUE2BIG;
-
-	for (insn = pos->begin; insn < pos->pos; insn++) {
-		struct bpf_insn *target;
-		u8 class = BPF_CLASS(insn->code);
-		u8 opcode;
-
-		if (class != BPF_JMP)
-			continue;
-		opcode = BPF_OP(insn->code);
-		if (opcode == BPF_CALL)
-			continue;
-
-		switch (insn->off) {
-		case JMP_TO_ERROR_CODE:
-			target = error_code;
-			break;
-		case JMP_TO_SUCCESS_CODE:
-			target = success_code;
-			break;
-		case JMP_TO_USER_CODE:
-			target = user_code;
-			break;
-		default:
-			pr_err("bpf prologue: internal error: relocation failed\n");
-			return -BPF_LOADER_ERRNO__PROLOGUE;
-		}
-
-		insn->off = target - (insn + 1);
-	}
-	return 0;
-}
-
-int bpf__gen_prologue(struct probe_trace_arg *args, int nargs,
-		      struct bpf_insn *new_prog, size_t *new_cnt,
-		      size_t cnt_space)
-{
-	struct bpf_insn *success_code = NULL;
-	struct bpf_insn *error_code = NULL;
-	struct bpf_insn *user_code = NULL;
-	struct bpf_insn_pos pos;
-	bool fastpath = true;
-	int err = 0, i;
-
-	if (!new_prog || !new_cnt)
-		return -EINVAL;
-
-	if (cnt_space > BPF_MAXINSNS)
-		cnt_space = BPF_MAXINSNS;
-
-	pos.begin = new_prog;
-	pos.end = new_prog + cnt_space;
-	pos.pos = new_prog;
-
-	if (!nargs) {
-		ins(BPF_ALU64_IMM(BPF_MOV, BPF_PROLOGUE_FETCH_RESULT_REG, 0),
-		    &pos);
-
-		if (check_pos(&pos))
-			goto errout;
-
-		*new_cnt = pos_get_cnt(&pos);
-		return 0;
-	}
-
-	if (nargs > BPF_PROLOGUE_MAX_ARGS) {
-		pr_warning("bpf: prologue: %d arguments are dropped\n",
-			   nargs - BPF_PROLOGUE_MAX_ARGS);
-		nargs = BPF_PROLOGUE_MAX_ARGS;
-	}
-
-	/* First pass: validation */
-	for (i = 0; i < nargs; i++) {
-		struct probe_trace_arg_ref *ref = args[i].ref;
-
-		if (args[i].value[0] == '@') {
-			/* TODO: fetch global variable */
-			pr_err("bpf: prologue: global %s%+ld not support\n",
-				args[i].value, ref ? ref->offset : 0);
-			return -ENOTSUP;
-		}
-
-		while (ref) {
-			/* fastpath is true if all args has ref == NULL */
-			fastpath = false;
-
-			/*
-			 * Instruction encodes immediate value using
-			 * s32, ref->offset is long. On systems which
-			 * can't fill long in s32, refuse to process if
-			 * ref->offset too large (or small).
-			 */
-#ifdef __LP64__
-#define OFFSET_MAX	((1LL << 31) - 1)
-#define OFFSET_MIN	((1LL << 31) * -1)
-			if (ref->offset > OFFSET_MAX ||
-					ref->offset < OFFSET_MIN) {
-				pr_err("bpf: prologue: offset out of bound: %ld\n",
-				       ref->offset);
-				return -BPF_LOADER_ERRNO__PROLOGUEOOB;
-			}
-#endif
-			ref = ref->next;
-		}
-	}
-	pr_debug("prologue: pass validation\n");
-
-	if (fastpath) {
-		/* If all variables are registers... */
-		pr_debug("prologue: fast path\n");
-		err = gen_prologue_fastpath(&pos, args, nargs);
-		if (err)
-			goto errout;
-	} else {
-		pr_debug("prologue: slow path\n");
-
-		/* Initialization: move ctx to a callee saved register. */
-		ins(BPF_MOV64_REG(BPF_REG_CTX, BPF_REG_ARG1), &pos);
-
-		err = gen_prologue_slowpath(&pos, args, nargs);
-		if (err)
-			goto errout;
-		/*
-		 * start of ERROR_CODE (only slow pass needs error code)
-		 *   mov r2 <- 1  // r2 is error number
-		 *   mov r3 <- 0  // r3, r4... should be touched or
-		 *                // verifier would complain
-		 *   mov r4 <- 0
-		 *   ...
-		 *   goto usercode
-		 */
-		error_code = pos.pos;
-		ins(BPF_ALU64_IMM(BPF_MOV, BPF_PROLOGUE_FETCH_RESULT_REG, 1),
-		    &pos);
-
-		for (i = 0; i < nargs; i++)
-			ins(BPF_ALU64_IMM(BPF_MOV,
-					  BPF_PROLOGUE_START_ARG_REG + i,
-					  0),
-			    &pos);
-		ins(BPF_JMP_IMM(BPF_JA, BPF_REG_0, 0, JMP_TO_USER_CODE),
-				&pos);
-	}
-
-	/*
-	 * start of SUCCESS_CODE:
-	 *   mov r2 <- 0
-	 *   goto usercode  // skip
-	 */
-	success_code = pos.pos;
-	ins(BPF_ALU64_IMM(BPF_MOV, BPF_PROLOGUE_FETCH_RESULT_REG, 0), &pos);
-
-	/*
-	 * start of USER_CODE:
-	 *   Restore ctx to r1
-	 */
-	user_code = pos.pos;
-	if (!fastpath) {
-		/*
-		 * Only slow path needs restoring of ctx. In fast path,
-		 * register are loaded directly from r1.
-		 */
-		ins(BPF_MOV64_REG(BPF_REG_ARG1, BPF_REG_CTX), &pos);
-		err = prologue_relocate(&pos, error_code, success_code,
-					user_code);
-		if (err)
-			goto errout;
-	}
-
-	err = check_pos(&pos);
-	if (err)
-		goto errout;
-
-	*new_cnt = pos_get_cnt(&pos);
-	return 0;
-errout:
-	return err;
-}


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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2022-01-17  9:25                       ` Jiri Olsa
@ 2022-01-18 23:05                         ` Andrii Nakryiko
  2022-01-22 20:29                         ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-01-18 23:05 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, Christy Lee,
	Christy Lee, bpf, linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

On Mon, Jan 17, 2022 at 1:26 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Jan 14, 2022 at 01:00:45PM -0800, Andrii Nakryiko wrote:
> > On Thu, Jan 13, 2022 at 7:14 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Thu, Jan 06, 2022 at 09:54:38AM -0800, Christy Lee wrote:
> > > > Thank you so much, I was able to reproduce the original tests after applying
> > > > the bug fix. I will submit a new patch set with the more detailed comments.
> > > >
> > > > The only deprecated functions that need to be removed after this would be
> > > > bpf_program__set_prep() (how perf sets the bpf prologue) and
> > > > bpf_program__nth_fd() (how perf leverages multi instance bpf). They look a
> > > > little more involved and I'm not sure how to approach those. Jiri, would you
> > > > mind taking a look at those please?
> > >
> > > hi,
> > > I checked and here's the way perf uses this interface:
> > >
> > >   - when bpf object/sources is specified on perf command line
> > >     we use bpf_object__open to load it
> > >
> > >   - user can define parameters in the section name for each bpf program
> > >     like:
> > >
> > >       SEC("lock_page=__lock_page page->flags")
> > >       int lock_page(struct pt_regs *ctx, int err, unsigned long flags)
> > >       {
> > >              return 1;
> > >       }
> > >
> > >     which tells perf to 'prepare' some extra bpf code for the program,
> > >     like to put value of 'page->flags' into 'flags' argument above
> > >
> > >   - perf generates extra prologue code to retrieve this data and does
> > >     that before the program is loaded by using bpf_program__set_prep
> > >     callback
> > >
> > >   - now the reason why we use bpf_program__set_prep for that, is because
> > >     it allows to create multiple instances for one bpf program
> > >
> > >   - we need multiple instances for single program, because probe can
> > >     result in multiple attach addresses (like for inlined functions)
> > >     with possible different ways of getting the arguments we need
> > >     to load
> > >
> > > I guess you want to get rid of that whole 'struct instances' related
> > > stuff, is that right?
> > >
> > > perf would need to load all the needed instances for program manually
> > > and somehow bypass/workaround bpf_object__load.. is there a way to
> > > manually add extra programs to bpf_object?
> > >
> > > thoughts? ;-)
> >
> > Sigh..
> >
> > 1. SEC("lock_page=__lock_page page->flags") will break in libbpf 1.0.
> > I'm going to add a way to provide a custom callback to handle such BPF
> > program sections by your custom code, but... Who's using this? Is
> > anyone using this? How is this used and for what? Would it be possible
> > to just kill this feature?
>
> good question ;-) IMO it was added in the early ebpf times, when nobody
> knew what will become the preferred way of doing things
>
> we don't know if there are any users of this, but:
>
> I had to go through the code to find out how to use it and it was broken
> in perf trace for some time while nobody complained ;-) also I don't think
> this is advertised anywhere in the doc
>
> Arnaldo,
> thoughts on removing this? ;-) I tried with the quick patch below, and

The fact that it was broken and no one complained and amount of
removed lines (not even talking about deprecated libbpf APIs) makes me
heavily +1'ing this! :) Thanks for performing code archeology!

> the standard perf trace ebpf support won't be affected by this
>
> the patch is removing the support for generating the ebpf program prologue
> which includes the usage of libbpf's instances APIs
>
> we could also remove the special section config parsing, which is used
> by prologue generation code
>
> jirka
>
>
> ---
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index 96ad944ca6a8..d9ff537d999e 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -556,17 +556,6 @@ ifndef NO_LIBELF
>        endif
>      endif
>
> -    ifndef NO_DWARF
> -      ifdef PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET
> -        CFLAGS += -DHAVE_BPF_PROLOGUE
> -        $(call detected,CONFIG_BPF_PROLOGUE)
> -      else
> -        msg := $(warning BPF prologue is not supported by architecture $(SRCARCH), missing regs_query_register_offset());
> -      endif
> -    else
> -      msg := $(warning DWARF support is off, BPF prologue is disabled);
> -    endif
> -
>    endif # NO_LIBBPF
>  endif # NO_LIBELF
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 6ac2160913ea..a04c02aed4c7 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -2685,20 +2685,6 @@ int cmd_record(int argc, const char **argv)
>         set_nobuild('\0', "clang-path", true);
>         set_nobuild('\0', "clang-opt", true);
>  # undef set_nobuild
> -#endif
> -
> -#ifndef HAVE_BPF_PROLOGUE
> -# if !defined (HAVE_DWARF_SUPPORT)
> -#  define REASON  "NO_DWARF=1"
> -# elif !defined (HAVE_LIBBPF_SUPPORT)
> -#  define REASON  "NO_LIBBPF=1"
> -# else
> -#  define REASON  "this architecture doesn't support BPF prologue"
> -# endif
> -# define set_nobuild(s, l, c) set_option_nobuild(record_options, s, l, REASON, c)
> -       set_nobuild('\0', "vmlinux", true);
> -# undef set_nobuild
> -# undef REASON
>  #endif
>
>         rec->opts.affinity = PERF_AFFINITY_SYS;
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index 22662fc85cc9..0e3f24dfee2c 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -40,10 +40,6 @@ struct bpf_prog_priv {
>         char *sys_name;
>         char *evt_name;
>         struct perf_probe_event pev;
> -       bool need_prologue;
> -       struct bpf_insn *insns_buf;
> -       int nr_types;
> -       int *type_mapping;
>  };
>
>  static bool libbpf_initialized;
> @@ -125,8 +121,6 @@ clear_prog_priv(struct bpf_program *prog __maybe_unused,
>         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);
> @@ -409,220 +403,6 @@ static int bpf__prepare_probe(void)
>         return err;
>  }
>
> -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)
> -{
> -       struct bpf_prog_priv *priv = bpf_program__priv(prog);
> -       struct probe_trace_event *tev;
> -       struct perf_probe_event *pev;
> -       struct bpf_insn *buf;
> -       size_t prologue_cnt = 0;
> -       int i, err;
> -
> -       if (IS_ERR_OR_NULL(priv) || priv->is_tp)
> -               goto errout;
> -
> -       pev = &priv->pev;
> -
> -       if (n < 0 || n >= priv->nr_types)
> -               goto errout;
> -
> -       /* Find a tev belongs to that type */
> -       for (i = 0; i < pev->ntevs; i++) {
> -               if (priv->type_mapping[i] == n)
> -                       break;
> -       }
> -
> -       if (i >= pev->ntevs) {
> -               pr_debug("Internal error: prologue type %d not found\n", n);
> -               return -BPF_LOADER_ERRNO__PROLOGUE;
> -       }
> -
> -       tev = &pev->tevs[i];
> -
> -       buf = priv->insns_buf;
> -       err = bpf__gen_prologue(tev->args, tev->nargs,
> -                               buf, &prologue_cnt,
> -                               BPF_MAXINSNS - orig_insns_cnt);
> -       if (err) {
> -               const char *title;
> -
> -               title = bpf_program__section_name(prog);
> -               pr_debug("Failed to generate prologue for program %s\n",
> -                        title);
> -               return err;
> -       }
> -
> -       memcpy(&buf[prologue_cnt], orig_insns,
> -              sizeof(struct bpf_insn) * orig_insns_cnt);
> -
> -       res->new_insn_ptr = buf;
> -       res->new_insn_cnt = prologue_cnt + orig_insns_cnt;
> -       res->pfd = NULL;
> -       return 0;
> -
> -errout:
> -       pr_debug("Internal error in preproc_gen_prologue\n");
> -       return -BPF_LOADER_ERRNO__PROLOGUE;
> -}
> -
> -/*
> - * compare_tev_args is reflexive, transitive and antisymmetric.
> - * I can proof it but this margin is too narrow to contain.
> - */
> -static int compare_tev_args(const void *ptev1, const void *ptev2)
> -{
> -       int i, ret;
> -       const struct probe_trace_event *tev1 =
> -               *(const struct probe_trace_event **)ptev1;
> -       const struct probe_trace_event *tev2 =
> -               *(const struct probe_trace_event **)ptev2;
> -
> -       ret = tev2->nargs - tev1->nargs;
> -       if (ret)
> -               return ret;
> -
> -       for (i = 0; i < tev1->nargs; i++) {
> -               struct probe_trace_arg *arg1, *arg2;
> -               struct probe_trace_arg_ref *ref1, *ref2;
> -
> -               arg1 = &tev1->args[i];
> -               arg2 = &tev2->args[i];
> -
> -               ret = strcmp(arg1->value, arg2->value);
> -               if (ret)
> -                       return ret;
> -
> -               ref1 = arg1->ref;
> -               ref2 = arg2->ref;
> -
> -               while (ref1 && ref2) {
> -                       ret = ref2->offset - ref1->offset;
> -                       if (ret)
> -                               return ret;
> -
> -                       ref1 = ref1->next;
> -                       ref2 = ref2->next;
> -               }
> -
> -               if (ref1 || ref2)
> -                       return ref2 ? 1 : -1;
> -       }
> -
> -       return 0;
> -}
> -
> -/*
> - * Assign a type number to each tevs in a pev.
> - * mapping is an array with same slots as tevs in that pev.
> - * nr_types will be set to number of types.
> - */
> -static int map_prologue(struct perf_probe_event *pev, int *mapping,
> -                       int *nr_types)
> -{
> -       int i, type = 0;
> -       struct probe_trace_event **ptevs;
> -
> -       size_t array_sz = sizeof(*ptevs) * pev->ntevs;
> -
> -       ptevs = malloc(array_sz);
> -       if (!ptevs) {
> -               pr_debug("Not enough memory: alloc ptevs failed\n");
> -               return -ENOMEM;
> -       }
> -
> -       pr_debug("In map_prologue, ntevs=%d\n", pev->ntevs);
> -       for (i = 0; i < pev->ntevs; i++)
> -               ptevs[i] = &pev->tevs[i];
> -
> -       qsort(ptevs, pev->ntevs, sizeof(*ptevs),
> -             compare_tev_args);
> -
> -       for (i = 0; i < pev->ntevs; i++) {
> -               int n;
> -
> -               n = ptevs[i] - pev->tevs;
> -               if (i == 0) {
> -                       mapping[n] = type;
> -                       pr_debug("mapping[%d]=%d\n", n, type);
> -                       continue;
> -               }
> -
> -               if (compare_tev_args(ptevs + i, ptevs + i - 1) == 0)
> -                       mapping[n] = type;
> -               else
> -                       mapping[n] = ++type;
> -
> -               pr_debug("mapping[%d]=%d\n", n, mapping[n]);
> -       }
> -       free(ptevs);
> -       *nr_types = type + 1;
> -
> -       return 0;
> -}
> -
> -static int hook_load_preprocessor(struct bpf_program *prog)
> -{
> -       struct bpf_prog_priv *priv = bpf_program__priv(prog);
> -       struct perf_probe_event *pev;
> -       bool need_prologue = false;
> -       int err, i;
> -
> -       if (IS_ERR_OR_NULL(priv)) {
> -               pr_debug("Internal error when hook preprocessor\n");
> -               return -BPF_LOADER_ERRNO__INTERNAL;
> -       }
> -
> -       if (priv->is_tp) {
> -               priv->need_prologue = false;
> -               return 0;
> -       }
> -
> -       pev = &priv->pev;
> -       for (i = 0; i < pev->ntevs; i++) {
> -               struct probe_trace_event *tev = &pev->tevs[i];
> -
> -               if (tev->nargs > 0) {
> -                       need_prologue = true;
> -                       break;
> -               }
> -       }
> -
> -       /*
> -        * Since all tevs don't have argument, we don't need generate
> -        * prologue.
> -        */
> -       if (!need_prologue) {
> -               priv->need_prologue = false;
> -               return 0;
> -       }
> -
> -       priv->need_prologue = true;
> -       priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
> -       if (!priv->insns_buf) {
> -               pr_debug("Not enough memory: alloc insns_buf failed\n");
> -               return -ENOMEM;
> -       }
> -
> -       priv->type_mapping = malloc(sizeof(int) * pev->ntevs);
> -       if (!priv->type_mapping) {
> -               pr_debug("Not enough memory: alloc type_mapping failed\n");
> -               return -ENOMEM;
> -       }
> -       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;
> -}
> -
>  int bpf__probe(struct bpf_object *obj)
>  {
>         int err = 0;
> @@ -669,18 +449,6 @@ int bpf__probe(struct bpf_object *obj)
>                         pr_debug("bpf_probe: failed to apply perf probe events\n");
>                         goto out;
>                 }
> -
> -               /*
> -                * After probing, let's consider prologue, which
> -                * adds program fetcher to BPF programs.
> -                *
> -                * hook_load_preprocessor() hooks pre-processor
> -                * to bpf_program, let it generate prologue
> -                * dynamically during loading.
> -                */
> -               err = hook_load_preprocessor(prog);
> -               if (err)
> -                       goto out;
>         }
>  out:
>         return err < 0 ? err : 0;
> @@ -773,14 +541,7 @@ 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 {
> -                               fd = bpf_program__fd(prog);
> -                       }
> -
> +                       fd = bpf_program__fd(prog);
>                         if (fd < 0) {
>                                 pr_debug("bpf: failed to get file descriptor\n");
>                                 return fd;
> diff --git a/tools/perf/util/bpf-prologue.c b/tools/perf/util/bpf-prologue.c
> deleted file mode 100644
> index 9887ae09242d..000000000000
> --- a/tools/perf/util/bpf-prologue.c
> +++ /dev/null
> @@ -1,508 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * bpf-prologue.c
> - *
> - * Copyright (C) 2015 He Kuang <hekuang@huawei.com>
> - * Copyright (C) 2015 Wang Nan <wangnan0@huawei.com>
> - * Copyright (C) 2015 Huawei Inc.
> - */
> -
> -#include <bpf/libbpf.h>
> -#include "debug.h"
> -#include "bpf-loader.h"
> -#include "bpf-prologue.h"
> -#include "probe-finder.h"
> -#include <errno.h>
> -#include <stdlib.h>
> -#include <dwarf-regs.h>
> -#include <linux/filter.h>
> -
> -#define BPF_REG_SIZE           8
> -
> -#define JMP_TO_ERROR_CODE      -1
> -#define JMP_TO_SUCCESS_CODE    -2
> -#define JMP_TO_USER_CODE       -3
> -
> -struct bpf_insn_pos {
> -       struct bpf_insn *begin;
> -       struct bpf_insn *end;
> -       struct bpf_insn *pos;
> -};
> -
> -static inline int
> -pos_get_cnt(struct bpf_insn_pos *pos)
> -{
> -       return pos->pos - pos->begin;
> -}
> -
> -static int
> -append_insn(struct bpf_insn new_insn, struct bpf_insn_pos *pos)
> -{
> -       if (!pos->pos)
> -               return -BPF_LOADER_ERRNO__PROLOGUE2BIG;
> -
> -       if (pos->pos + 1 >= pos->end) {
> -               pr_err("bpf prologue: prologue too long\n");
> -               pos->pos = NULL;
> -               return -BPF_LOADER_ERRNO__PROLOGUE2BIG;
> -       }
> -
> -       *(pos->pos)++ = new_insn;
> -       return 0;
> -}
> -
> -static int
> -check_pos(struct bpf_insn_pos *pos)
> -{
> -       if (!pos->pos || pos->pos >= pos->end)
> -               return -BPF_LOADER_ERRNO__PROLOGUE2BIG;
> -       return 0;
> -}
> -
> -/*
> - * Convert type string (u8/u16/u32/u64/s8/s16/s32/s64 ..., see
> - * Documentation/trace/kprobetrace.rst) to size field of BPF_LDX_MEM
> - * instruction (BPF_{B,H,W,DW}).
> - */
> -static int
> -argtype_to_ldx_size(const char *type)
> -{
> -       int arg_size = type ? atoi(&type[1]) : 64;
> -
> -       switch (arg_size) {
> -       case 8:
> -               return BPF_B;
> -       case 16:
> -               return BPF_H;
> -       case 32:
> -               return BPF_W;
> -       case 64:
> -       default:
> -               return BPF_DW;
> -       }
> -}
> -
> -static const char *
> -insn_sz_to_str(int insn_sz)
> -{
> -       switch (insn_sz) {
> -       case BPF_B:
> -               return "BPF_B";
> -       case BPF_H:
> -               return "BPF_H";
> -       case BPF_W:
> -               return "BPF_W";
> -       case BPF_DW:
> -               return "BPF_DW";
> -       default:
> -               return "UNKNOWN";
> -       }
> -}
> -
> -/* Give it a shorter name */
> -#define ins(i, p) append_insn((i), (p))
> -
> -/*
> - * Give a register name (in 'reg'), generate instruction to
> - * load register into an eBPF register rd:
> - *   'ldd target_reg, offset(ctx_reg)', where:
> - * ctx_reg is pre initialized to pointer of 'struct pt_regs'.
> - */
> -static int
> -gen_ldx_reg_from_ctx(struct bpf_insn_pos *pos, int ctx_reg,
> -                    const char *reg, int target_reg)
> -{
> -       int offset = regs_query_register_offset(reg);
> -
> -       if (offset < 0) {
> -               pr_err("bpf: prologue: failed to get register %s\n",
> -                      reg);
> -               return offset;
> -       }
> -       ins(BPF_LDX_MEM(BPF_DW, target_reg, ctx_reg, offset), pos);
> -
> -       return check_pos(pos);
> -}
> -
> -/*
> - * Generate a BPF_FUNC_probe_read function call.
> - *
> - * src_base_addr_reg is a register holding base address,
> - * dst_addr_reg is a register holding dest address (on stack),
> - * result is:
> - *
> - *  *[dst_addr_reg] = *([src_base_addr_reg] + offset)
> - *
> - * Arguments of BPF_FUNC_probe_read:
> - *     ARG1: ptr to stack (dest)
> - *     ARG2: size (8)
> - *     ARG3: unsafe ptr (src)
> - */
> -static int
> -gen_read_mem(struct bpf_insn_pos *pos,
> -            int src_base_addr_reg,
> -            int dst_addr_reg,
> -            long offset,
> -            int probeid)
> -{
> -       /* mov arg3, src_base_addr_reg */
> -       if (src_base_addr_reg != BPF_REG_ARG3)
> -               ins(BPF_MOV64_REG(BPF_REG_ARG3, src_base_addr_reg), pos);
> -       /* add arg3, #offset */
> -       if (offset)
> -               ins(BPF_ALU64_IMM(BPF_ADD, BPF_REG_ARG3, offset), pos);
> -
> -       /* mov arg2, #reg_size */
> -       ins(BPF_ALU64_IMM(BPF_MOV, BPF_REG_ARG2, BPF_REG_SIZE), pos);
> -
> -       /* mov arg1, dst_addr_reg */
> -       if (dst_addr_reg != BPF_REG_ARG1)
> -               ins(BPF_MOV64_REG(BPF_REG_ARG1, dst_addr_reg), pos);
> -
> -       /* Call probe_read  */
> -       ins(BPF_EMIT_CALL(probeid), pos);
> -       /*
> -        * Error processing: if read fail, goto error code,
> -        * will be relocated. Target should be the start of
> -        * error processing code.
> -        */
> -       ins(BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, JMP_TO_ERROR_CODE),
> -           pos);
> -
> -       return check_pos(pos);
> -}
> -
> -/*
> - * Each arg should be bare register. Fetch and save them into argument
> - * registers (r3 - r5).
> - *
> - * BPF_REG_1 should have been initialized with pointer to
> - * 'struct pt_regs'.
> - */
> -static int
> -gen_prologue_fastpath(struct bpf_insn_pos *pos,
> -                     struct probe_trace_arg *args, int nargs)
> -{
> -       int i, err = 0;
> -
> -       for (i = 0; i < nargs; i++) {
> -               err = gen_ldx_reg_from_ctx(pos, BPF_REG_1, args[i].value,
> -                                          BPF_PROLOGUE_START_ARG_REG + i);
> -               if (err)
> -                       goto errout;
> -       }
> -
> -       return check_pos(pos);
> -errout:
> -       return err;
> -}
> -
> -/*
> - * Slow path:
> - *   At least one argument has the form of 'offset($rx)'.
> - *
> - * Following code first stores them into stack, then loads all of then
> - * to r2 - r5.
> - * Before final loading, the final result should be:
> - *
> - * low address
> - * BPF_REG_FP - 24  ARG3
> - * BPF_REG_FP - 16  ARG2
> - * BPF_REG_FP - 8   ARG1
> - * BPF_REG_FP
> - * high address
> - *
> - * For each argument (described as: offn(...off2(off1(reg)))),
> - * generates following code:
> - *
> - *  r7 <- fp
> - *  r7 <- r7 - stack_offset  // Ideal code should initialize r7 using
> - *                           // fp before generating args. However,
> - *                           // eBPF won't regard r7 as stack pointer
> - *                           // if it is generated by minus 8 from
> - *                           // another stack pointer except fp.
> - *                           // This is why we have to set r7
> - *                           // to fp for each variable.
> - *  r3 <- value of 'reg'-> generated using gen_ldx_reg_from_ctx()
> - *  (r7) <- r3       // skip following instructions for bare reg
> - *  r3 <- r3 + off1  . // skip if off1 == 0
> - *  r2 <- 8           \
> - *  r1 <- r7           |-> generated by gen_read_mem()
> - *  call probe_read    /
> - *  jnei r0, 0, err  ./
> - *  r3 <- (r7)
> - *  r3 <- r3 + off2  . // skip if off2 == 0
> - *  r2 <- 8           \  // r2 may be broken by probe_read, so set again
> - *  r1 <- r7           |-> generated by gen_read_mem()
> - *  call probe_read    /
> - *  jnei r0, 0, err  ./
> - *  ...
> - */
> -static int
> -gen_prologue_slowpath(struct bpf_insn_pos *pos,
> -                     struct probe_trace_arg *args, int nargs)
> -{
> -       int err, i, probeid;
> -
> -       for (i = 0; i < nargs; i++) {
> -               struct probe_trace_arg *arg = &args[i];
> -               const char *reg = arg->value;
> -               struct probe_trace_arg_ref *ref = NULL;
> -               int stack_offset = (i + 1) * -8;
> -
> -               pr_debug("prologue: fetch arg %d, base reg is %s\n",
> -                        i, reg);
> -
> -               /* value of base register is stored into ARG3 */
> -               err = gen_ldx_reg_from_ctx(pos, BPF_REG_CTX, reg,
> -                                          BPF_REG_ARG3);
> -               if (err) {
> -                       pr_err("prologue: failed to get offset of register %s\n",
> -                              reg);
> -                       goto errout;
> -               }
> -
> -               /* Make r7 the stack pointer. */
> -               ins(BPF_MOV64_REG(BPF_REG_7, BPF_REG_FP), pos);
> -               /* r7 += -8 */
> -               ins(BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, stack_offset), pos);
> -               /*
> -                * Store r3 (base register) onto stack
> -                * Ensure fp[offset] is set.
> -                * fp is the only valid base register when storing
> -                * into stack. We are not allowed to use r7 as base
> -                * register here.
> -                */
> -               ins(BPF_STX_MEM(BPF_DW, BPF_REG_FP, BPF_REG_ARG3,
> -                               stack_offset), pos);
> -
> -               ref = arg->ref;
> -               probeid = BPF_FUNC_probe_read_kernel;
> -               while (ref) {
> -                       pr_debug("prologue: arg %d: offset %ld\n",
> -                                i, ref->offset);
> -
> -                       if (ref->user_access)
> -                               probeid = BPF_FUNC_probe_read_user;
> -
> -                       err = gen_read_mem(pos, BPF_REG_3, BPF_REG_7,
> -                                          ref->offset, probeid);
> -                       if (err) {
> -                               pr_err("prologue: failed to generate probe_read function call\n");
> -                               goto errout;
> -                       }
> -
> -                       ref = ref->next;
> -                       /*
> -                        * Load previous result into ARG3. Use
> -                        * BPF_REG_FP instead of r7 because verifier
> -                        * allows FP based addressing only.
> -                        */
> -                       if (ref)
> -                               ins(BPF_LDX_MEM(BPF_DW, BPF_REG_ARG3,
> -                                               BPF_REG_FP, stack_offset), pos);
> -               }
> -       }
> -
> -       /* Final pass: read to registers */
> -       for (i = 0; i < nargs; i++) {
> -               int insn_sz = (args[i].ref) ? argtype_to_ldx_size(args[i].type) : BPF_DW;
> -
> -               pr_debug("prologue: load arg %d, insn_sz is %s\n",
> -                        i, insn_sz_to_str(insn_sz));
> -               ins(BPF_LDX_MEM(insn_sz, BPF_PROLOGUE_START_ARG_REG + i,
> -                               BPF_REG_FP, -BPF_REG_SIZE * (i + 1)), pos);
> -       }
> -
> -       ins(BPF_JMP_IMM(BPF_JA, BPF_REG_0, 0, JMP_TO_SUCCESS_CODE), pos);
> -
> -       return check_pos(pos);
> -errout:
> -       return err;
> -}
> -
> -static int
> -prologue_relocate(struct bpf_insn_pos *pos, struct bpf_insn *error_code,
> -                 struct bpf_insn *success_code, struct bpf_insn *user_code)
> -{
> -       struct bpf_insn *insn;
> -
> -       if (check_pos(pos))
> -               return -BPF_LOADER_ERRNO__PROLOGUE2BIG;
> -
> -       for (insn = pos->begin; insn < pos->pos; insn++) {
> -               struct bpf_insn *target;
> -               u8 class = BPF_CLASS(insn->code);
> -               u8 opcode;
> -
> -               if (class != BPF_JMP)
> -                       continue;
> -               opcode = BPF_OP(insn->code);
> -               if (opcode == BPF_CALL)
> -                       continue;
> -
> -               switch (insn->off) {
> -               case JMP_TO_ERROR_CODE:
> -                       target = error_code;
> -                       break;
> -               case JMP_TO_SUCCESS_CODE:
> -                       target = success_code;
> -                       break;
> -               case JMP_TO_USER_CODE:
> -                       target = user_code;
> -                       break;
> -               default:
> -                       pr_err("bpf prologue: internal error: relocation failed\n");
> -                       return -BPF_LOADER_ERRNO__PROLOGUE;
> -               }
> -
> -               insn->off = target - (insn + 1);
> -       }
> -       return 0;
> -}
> -
> -int bpf__gen_prologue(struct probe_trace_arg *args, int nargs,
> -                     struct bpf_insn *new_prog, size_t *new_cnt,
> -                     size_t cnt_space)
> -{
> -       struct bpf_insn *success_code = NULL;
> -       struct bpf_insn *error_code = NULL;
> -       struct bpf_insn *user_code = NULL;
> -       struct bpf_insn_pos pos;
> -       bool fastpath = true;
> -       int err = 0, i;
> -
> -       if (!new_prog || !new_cnt)
> -               return -EINVAL;
> -
> -       if (cnt_space > BPF_MAXINSNS)
> -               cnt_space = BPF_MAXINSNS;
> -
> -       pos.begin = new_prog;
> -       pos.end = new_prog + cnt_space;
> -       pos.pos = new_prog;
> -
> -       if (!nargs) {
> -               ins(BPF_ALU64_IMM(BPF_MOV, BPF_PROLOGUE_FETCH_RESULT_REG, 0),
> -                   &pos);
> -
> -               if (check_pos(&pos))
> -                       goto errout;
> -
> -               *new_cnt = pos_get_cnt(&pos);
> -               return 0;
> -       }
> -
> -       if (nargs > BPF_PROLOGUE_MAX_ARGS) {
> -               pr_warning("bpf: prologue: %d arguments are dropped\n",
> -                          nargs - BPF_PROLOGUE_MAX_ARGS);
> -               nargs = BPF_PROLOGUE_MAX_ARGS;
> -       }
> -
> -       /* First pass: validation */
> -       for (i = 0; i < nargs; i++) {
> -               struct probe_trace_arg_ref *ref = args[i].ref;
> -
> -               if (args[i].value[0] == '@') {
> -                       /* TODO: fetch global variable */
> -                       pr_err("bpf: prologue: global %s%+ld not support\n",
> -                               args[i].value, ref ? ref->offset : 0);
> -                       return -ENOTSUP;
> -               }
> -
> -               while (ref) {
> -                       /* fastpath is true if all args has ref == NULL */
> -                       fastpath = false;
> -
> -                       /*
> -                        * Instruction encodes immediate value using
> -                        * s32, ref->offset is long. On systems which
> -                        * can't fill long in s32, refuse to process if
> -                        * ref->offset too large (or small).
> -                        */
> -#ifdef __LP64__
> -#define OFFSET_MAX     ((1LL << 31) - 1)
> -#define OFFSET_MIN     ((1LL << 31) * -1)
> -                       if (ref->offset > OFFSET_MAX ||
> -                                       ref->offset < OFFSET_MIN) {
> -                               pr_err("bpf: prologue: offset out of bound: %ld\n",
> -                                      ref->offset);
> -                               return -BPF_LOADER_ERRNO__PROLOGUEOOB;
> -                       }
> -#endif
> -                       ref = ref->next;
> -               }
> -       }
> -       pr_debug("prologue: pass validation\n");
> -
> -       if (fastpath) {
> -               /* If all variables are registers... */
> -               pr_debug("prologue: fast path\n");
> -               err = gen_prologue_fastpath(&pos, args, nargs);
> -               if (err)
> -                       goto errout;
> -       } else {
> -               pr_debug("prologue: slow path\n");
> -
> -               /* Initialization: move ctx to a callee saved register. */
> -               ins(BPF_MOV64_REG(BPF_REG_CTX, BPF_REG_ARG1), &pos);
> -
> -               err = gen_prologue_slowpath(&pos, args, nargs);
> -               if (err)
> -                       goto errout;
> -               /*
> -                * start of ERROR_CODE (only slow pass needs error code)
> -                *   mov r2 <- 1  // r2 is error number
> -                *   mov r3 <- 0  // r3, r4... should be touched or
> -                *                // verifier would complain
> -                *   mov r4 <- 0
> -                *   ...
> -                *   goto usercode
> -                */
> -               error_code = pos.pos;
> -               ins(BPF_ALU64_IMM(BPF_MOV, BPF_PROLOGUE_FETCH_RESULT_REG, 1),
> -                   &pos);
> -
> -               for (i = 0; i < nargs; i++)
> -                       ins(BPF_ALU64_IMM(BPF_MOV,
> -                                         BPF_PROLOGUE_START_ARG_REG + i,
> -                                         0),
> -                           &pos);
> -               ins(BPF_JMP_IMM(BPF_JA, BPF_REG_0, 0, JMP_TO_USER_CODE),
> -                               &pos);
> -       }
> -
> -       /*
> -        * start of SUCCESS_CODE:
> -        *   mov r2 <- 0
> -        *   goto usercode  // skip
> -        */
> -       success_code = pos.pos;
> -       ins(BPF_ALU64_IMM(BPF_MOV, BPF_PROLOGUE_FETCH_RESULT_REG, 0), &pos);
> -
> -       /*
> -        * start of USER_CODE:
> -        *   Restore ctx to r1
> -        */
> -       user_code = pos.pos;
> -       if (!fastpath) {
> -               /*
> -                * Only slow path needs restoring of ctx. In fast path,
> -                * register are loaded directly from r1.
> -                */
> -               ins(BPF_MOV64_REG(BPF_REG_ARG1, BPF_REG_CTX), &pos);
> -               err = prologue_relocate(&pos, error_code, success_code,
> -                                       user_code);
> -               if (err)
> -                       goto errout;
> -       }
> -
> -       err = check_pos(&pos);
> -       if (err)
> -               goto errout;
> -
> -       *new_cnt = pos_get_cnt(&pos);
> -       return 0;
> -errout:
> -       return err;
> -}
>

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

* Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API
  2022-01-17  9:25                       ` Jiri Olsa
  2022-01-18 23:05                         ` Andrii Nakryiko
@ 2022-01-22 20:29                         ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 18+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-01-22 20:29 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Andrii Nakryiko, Andrii Nakryiko, Christy Lee, Christy Lee, bpf,
	linux-perf-use.,
	Kernel Team, He Kuang, Wang Nan, Wang ShaoBo, YueHaibing

Em Mon, Jan 17, 2022 at 10:25:59AM +0100, Jiri Olsa escreveu:
> On Fri, Jan 14, 2022 at 01:00:45PM -0800, Andrii Nakryiko wrote:
> > On Thu, Jan 13, 2022 at 7:14 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Thu, Jan 06, 2022 at 09:54:38AM -0800, Christy Lee wrote:
> > > > Thank you so much, I was able to reproduce the original tests after applying
> > > > the bug fix. I will submit a new patch set with the more detailed comments.
> > > >
> > > > The only deprecated functions that need to be removed after this would be
> > > > bpf_program__set_prep() (how perf sets the bpf prologue) and
> > > > bpf_program__nth_fd() (how perf leverages multi instance bpf). They look a
> > > > little more involved and I'm not sure how to approach those. Jiri, would you
> > > > mind taking a look at those please?
> > >
> > > hi,
> > > I checked and here's the way perf uses this interface:
> > >
> > >   - when bpf object/sources is specified on perf command line
> > >     we use bpf_object__open to load it
> > >
> > >   - user can define parameters in the section name for each bpf program
> > >     like:
> > >
> > >       SEC("lock_page=__lock_page page->flags")
> > >       int lock_page(struct pt_regs *ctx, int err, unsigned long flags)
> > >       {
> > >              return 1;
> > >       }
> > >
> > >     which tells perf to 'prepare' some extra bpf code for the program,
> > >     like to put value of 'page->flags' into 'flags' argument above
> > >
> > >   - perf generates extra prologue code to retrieve this data and does
> > >     that before the program is loaded by using bpf_program__set_prep
> > >     callback
> > >
> > >   - now the reason why we use bpf_program__set_prep for that, is because
> > >     it allows to create multiple instances for one bpf program
> > >
> > >   - we need multiple instances for single program, because probe can
> > >     result in multiple attach addresses (like for inlined functions)
> > >     with possible different ways of getting the arguments we need
> > >     to load
> > >
> > > I guess you want to get rid of that whole 'struct instances' related
> > > stuff, is that right?
> > >
> > > perf would need to load all the needed instances for program manually
> > > and somehow bypass/workaround bpf_object__load.. is there a way to
> > > manually add extra programs to bpf_object?
> > >
> > > thoughts? ;-)
> > 
> > Sigh..
> > 
> > 1. SEC("lock_page=__lock_page page->flags") will break in libbpf 1.0.
> > I'm going to add a way to provide a custom callback to handle such BPF
> > program sections by your custom code, but... Who's using this? Is
> > anyone using this? How is this used and for what? Would it be possible
> > to just kill this feature?
> 
> good question ;-) IMO it was added in the early ebpf times, when nobody
> knew what will become the preferred way of doing things
> 
> we don't know if there are any users of this, but:
> 
> I had to go through the code to find out how to use it and it was broken
> in perf trace for some time while nobody complained ;-) also I don't think
> this is advertised anywhere in the doc
> 
> Arnaldo,
> thoughts on removing this? ;-) I tried with the quick patch below, and
> the standard perf trace ebpf support won't be affected by this
> 
> the patch is removing the support for generating the ebpf program prologue
> which includes the usage of libbpf's instances APIs
> 
> we could also remove the special section config parsing, which is used
> by prologue generation code

This was all done a long time ago, mostly by Wang Nan, so if you tested
it based on the committer testing comments, etc, and everything seems to
work...

I'll try and give it a go after pushing the current lot to Linus.

- Arnaldo
 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index 96ad944ca6a8..d9ff537d999e 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -556,17 +556,6 @@ ifndef NO_LIBELF
>        endif
>      endif
>  
> -    ifndef NO_DWARF
> -      ifdef PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET
> -        CFLAGS += -DHAVE_BPF_PROLOGUE
> -        $(call detected,CONFIG_BPF_PROLOGUE)
> -      else
> -        msg := $(warning BPF prologue is not supported by architecture $(SRCARCH), missing regs_query_register_offset());
> -      endif
> -    else
> -      msg := $(warning DWARF support is off, BPF prologue is disabled);
> -    endif
> -
>    endif # NO_LIBBPF
>  endif # NO_LIBELF
>  
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 6ac2160913ea..a04c02aed4c7 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -2685,20 +2685,6 @@ int cmd_record(int argc, const char **argv)
>  	set_nobuild('\0', "clang-path", true);
>  	set_nobuild('\0', "clang-opt", true);
>  # undef set_nobuild
> -#endif
> -
> -#ifndef HAVE_BPF_PROLOGUE
> -# if !defined (HAVE_DWARF_SUPPORT)
> -#  define REASON  "NO_DWARF=1"
> -# elif !defined (HAVE_LIBBPF_SUPPORT)
> -#  define REASON  "NO_LIBBPF=1"
> -# else
> -#  define REASON  "this architecture doesn't support BPF prologue"
> -# endif
> -# define set_nobuild(s, l, c) set_option_nobuild(record_options, s, l, REASON, c)
> -	set_nobuild('\0', "vmlinux", true);
> -# undef set_nobuild
> -# undef REASON
>  #endif
>  
>  	rec->opts.affinity = PERF_AFFINITY_SYS;
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index 22662fc85cc9..0e3f24dfee2c 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -40,10 +40,6 @@ struct bpf_prog_priv {
>  	char *sys_name;
>  	char *evt_name;
>  	struct perf_probe_event pev;
> -	bool need_prologue;
> -	struct bpf_insn *insns_buf;
> -	int nr_types;
> -	int *type_mapping;
>  };
>  
>  static bool libbpf_initialized;
> @@ -125,8 +121,6 @@ clear_prog_priv(struct bpf_program *prog __maybe_unused,
>  	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);
> @@ -409,220 +403,6 @@ static int bpf__prepare_probe(void)
>  	return err;
>  }
>  
> -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)
> -{
> -	struct bpf_prog_priv *priv = bpf_program__priv(prog);
> -	struct probe_trace_event *tev;
> -	struct perf_probe_event *pev;
> -	struct bpf_insn *buf;
> -	size_t prologue_cnt = 0;
> -	int i, err;
> -
> -	if (IS_ERR_OR_NULL(priv) || priv->is_tp)
> -		goto errout;
> -
> -	pev = &priv->pev;
> -
> -	if (n < 0 || n >= priv->nr_types)
> -		goto errout;
> -
> -	/* Find a tev belongs to that type */
> -	for (i = 0; i < pev->ntevs; i++) {
> -		if (priv->type_mapping[i] == n)
> -			break;
> -	}
> -
> -	if (i >= pev->ntevs) {
> -		pr_debug("Internal error: prologue type %d not found\n", n);
> -		return -BPF_LOADER_ERRNO__PROLOGUE;
> -	}
> -
> -	tev = &pev->tevs[i];
> -
> -	buf = priv->insns_buf;
> -	err = bpf__gen_prologue(tev->args, tev->nargs,
> -				buf, &prologue_cnt,
> -				BPF_MAXINSNS - orig_insns_cnt);
> -	if (err) {
> -		const char *title;
> -
> -		title = bpf_program__section_name(prog);
> -		pr_debug("Failed to generate prologue for program %s\n",
> -			 title);
> -		return err;
> -	}
> -
> -	memcpy(&buf[prologue_cnt], orig_insns,
> -	       sizeof(struct bpf_insn) * orig_insns_cnt);
> -
> -	res->new_insn_ptr = buf;
> -	res->new_insn_cnt = prologue_cnt + orig_insns_cnt;
> -	res->pfd = NULL;
> -	return 0;
> -
> -errout:
> -	pr_debug("Internal error in preproc_gen_prologue\n");
> -	return -BPF_LOADER_ERRNO__PROLOGUE;
> -}
> -
> -/*
> - * compare_tev_args is reflexive, transitive and antisymmetric.
> - * I can proof it but this margin is too narrow to contain.
> - */
> -static int compare_tev_args(const void *ptev1, const void *ptev2)
> -{
> -	int i, ret;
> -	const struct probe_trace_event *tev1 =
> -		*(const struct probe_trace_event **)ptev1;
> -	const struct probe_trace_event *tev2 =
> -		*(const struct probe_trace_event **)ptev2;
> -
> -	ret = tev2->nargs - tev1->nargs;
> -	if (ret)
> -		return ret;
> -
> -	for (i = 0; i < tev1->nargs; i++) {
> -		struct probe_trace_arg *arg1, *arg2;
> -		struct probe_trace_arg_ref *ref1, *ref2;
> -
> -		arg1 = &tev1->args[i];
> -		arg2 = &tev2->args[i];
> -
> -		ret = strcmp(arg1->value, arg2->value);
> -		if (ret)
> -			return ret;
> -
> -		ref1 = arg1->ref;
> -		ref2 = arg2->ref;
> -
> -		while (ref1 && ref2) {
> -			ret = ref2->offset - ref1->offset;
> -			if (ret)
> -				return ret;
> -
> -			ref1 = ref1->next;
> -			ref2 = ref2->next;
> -		}
> -
> -		if (ref1 || ref2)
> -			return ref2 ? 1 : -1;
> -	}
> -
> -	return 0;
> -}
> -
> -/*
> - * Assign a type number to each tevs in a pev.
> - * mapping is an array with same slots as tevs in that pev.
> - * nr_types will be set to number of types.
> - */
> -static int map_prologue(struct perf_probe_event *pev, int *mapping,
> -			int *nr_types)
> -{
> -	int i, type = 0;
> -	struct probe_trace_event **ptevs;
> -
> -	size_t array_sz = sizeof(*ptevs) * pev->ntevs;
> -
> -	ptevs = malloc(array_sz);
> -	if (!ptevs) {
> -		pr_debug("Not enough memory: alloc ptevs failed\n");
> -		return -ENOMEM;
> -	}
> -
> -	pr_debug("In map_prologue, ntevs=%d\n", pev->ntevs);
> -	for (i = 0; i < pev->ntevs; i++)
> -		ptevs[i] = &pev->tevs[i];
> -
> -	qsort(ptevs, pev->ntevs, sizeof(*ptevs),
> -	      compare_tev_args);
> -
> -	for (i = 0; i < pev->ntevs; i++) {
> -		int n;
> -
> -		n = ptevs[i] - pev->tevs;
> -		if (i == 0) {
> -			mapping[n] = type;
> -			pr_debug("mapping[%d]=%d\n", n, type);
> -			continue;
> -		}
> -
> -		if (compare_tev_args(ptevs + i, ptevs + i - 1) == 0)
> -			mapping[n] = type;
> -		else
> -			mapping[n] = ++type;
> -
> -		pr_debug("mapping[%d]=%d\n", n, mapping[n]);
> -	}
> -	free(ptevs);
> -	*nr_types = type + 1;
> -
> -	return 0;
> -}
> -
> -static int hook_load_preprocessor(struct bpf_program *prog)
> -{
> -	struct bpf_prog_priv *priv = bpf_program__priv(prog);
> -	struct perf_probe_event *pev;
> -	bool need_prologue = false;
> -	int err, i;
> -
> -	if (IS_ERR_OR_NULL(priv)) {
> -		pr_debug("Internal error when hook preprocessor\n");
> -		return -BPF_LOADER_ERRNO__INTERNAL;
> -	}
> -
> -	if (priv->is_tp) {
> -		priv->need_prologue = false;
> -		return 0;
> -	}
> -
> -	pev = &priv->pev;
> -	for (i = 0; i < pev->ntevs; i++) {
> -		struct probe_trace_event *tev = &pev->tevs[i];
> -
> -		if (tev->nargs > 0) {
> -			need_prologue = true;
> -			break;
> -		}
> -	}
> -
> -	/*
> -	 * Since all tevs don't have argument, we don't need generate
> -	 * prologue.
> -	 */
> -	if (!need_prologue) {
> -		priv->need_prologue = false;
> -		return 0;
> -	}
> -
> -	priv->need_prologue = true;
> -	priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
> -	if (!priv->insns_buf) {
> -		pr_debug("Not enough memory: alloc insns_buf failed\n");
> -		return -ENOMEM;
> -	}
> -
> -	priv->type_mapping = malloc(sizeof(int) * pev->ntevs);
> -	if (!priv->type_mapping) {
> -		pr_debug("Not enough memory: alloc type_mapping failed\n");
> -		return -ENOMEM;
> -	}
> -	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;
> -}
> -
>  int bpf__probe(struct bpf_object *obj)
>  {
>  	int err = 0;
> @@ -669,18 +449,6 @@ int bpf__probe(struct bpf_object *obj)
>  			pr_debug("bpf_probe: failed to apply perf probe events\n");
>  			goto out;
>  		}
> -
> -		/*
> -		 * After probing, let's consider prologue, which
> -		 * adds program fetcher to BPF programs.
> -		 *
> -		 * hook_load_preprocessor() hooks pre-processor
> -		 * to bpf_program, let it generate prologue
> -		 * dynamically during loading.
> -		 */
> -		err = hook_load_preprocessor(prog);
> -		if (err)
> -			goto out;
>  	}
>  out:
>  	return err < 0 ? err : 0;
> @@ -773,14 +541,7 @@ 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 {
> -				fd = bpf_program__fd(prog);
> -			}
> -
> +			fd = bpf_program__fd(prog);
>  			if (fd < 0) {
>  				pr_debug("bpf: failed to get file descriptor\n");
>  				return fd;
> diff --git a/tools/perf/util/bpf-prologue.c b/tools/perf/util/bpf-prologue.c
> deleted file mode 100644
> index 9887ae09242d..000000000000
> --- a/tools/perf/util/bpf-prologue.c
> +++ /dev/null
> @@ -1,508 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * bpf-prologue.c
> - *
> - * Copyright (C) 2015 He Kuang <hekuang@huawei.com>
> - * Copyright (C) 2015 Wang Nan <wangnan0@huawei.com>
> - * Copyright (C) 2015 Huawei Inc.
> - */
> -
> -#include <bpf/libbpf.h>
> -#include "debug.h"
> -#include "bpf-loader.h"
> -#include "bpf-prologue.h"
> -#include "probe-finder.h"
> -#include <errno.h>
> -#include <stdlib.h>
> -#include <dwarf-regs.h>
> -#include <linux/filter.h>
> -
> -#define BPF_REG_SIZE		8
> -
> -#define JMP_TO_ERROR_CODE	-1
> -#define JMP_TO_SUCCESS_CODE	-2
> -#define JMP_TO_USER_CODE	-3
> -
> -struct bpf_insn_pos {
> -	struct bpf_insn *begin;
> -	struct bpf_insn *end;
> -	struct bpf_insn *pos;
> -};
> -
> -static inline int
> -pos_get_cnt(struct bpf_insn_pos *pos)
> -{
> -	return pos->pos - pos->begin;
> -}
> -
> -static int
> -append_insn(struct bpf_insn new_insn, struct bpf_insn_pos *pos)
> -{
> -	if (!pos->pos)
> -		return -BPF_LOADER_ERRNO__PROLOGUE2BIG;
> -
> -	if (pos->pos + 1 >= pos->end) {
> -		pr_err("bpf prologue: prologue too long\n");
> -		pos->pos = NULL;
> -		return -BPF_LOADER_ERRNO__PROLOGUE2BIG;
> -	}
> -
> -	*(pos->pos)++ = new_insn;
> -	return 0;
> -}
> -
> -static int
> -check_pos(struct bpf_insn_pos *pos)
> -{
> -	if (!pos->pos || pos->pos >= pos->end)
> -		return -BPF_LOADER_ERRNO__PROLOGUE2BIG;
> -	return 0;
> -}
> -
> -/*
> - * Convert type string (u8/u16/u32/u64/s8/s16/s32/s64 ..., see
> - * Documentation/trace/kprobetrace.rst) to size field of BPF_LDX_MEM
> - * instruction (BPF_{B,H,W,DW}).
> - */
> -static int
> -argtype_to_ldx_size(const char *type)
> -{
> -	int arg_size = type ? atoi(&type[1]) : 64;
> -
> -	switch (arg_size) {
> -	case 8:
> -		return BPF_B;
> -	case 16:
> -		return BPF_H;
> -	case 32:
> -		return BPF_W;
> -	case 64:
> -	default:
> -		return BPF_DW;
> -	}
> -}
> -
> -static const char *
> -insn_sz_to_str(int insn_sz)
> -{
> -	switch (insn_sz) {
> -	case BPF_B:
> -		return "BPF_B";
> -	case BPF_H:
> -		return "BPF_H";
> -	case BPF_W:
> -		return "BPF_W";
> -	case BPF_DW:
> -		return "BPF_DW";
> -	default:
> -		return "UNKNOWN";
> -	}
> -}
> -
> -/* Give it a shorter name */
> -#define ins(i, p) append_insn((i), (p))
> -
> -/*
> - * Give a register name (in 'reg'), generate instruction to
> - * load register into an eBPF register rd:
> - *   'ldd target_reg, offset(ctx_reg)', where:
> - * ctx_reg is pre initialized to pointer of 'struct pt_regs'.
> - */
> -static int
> -gen_ldx_reg_from_ctx(struct bpf_insn_pos *pos, int ctx_reg,
> -		     const char *reg, int target_reg)
> -{
> -	int offset = regs_query_register_offset(reg);
> -
> -	if (offset < 0) {
> -		pr_err("bpf: prologue: failed to get register %s\n",
> -		       reg);
> -		return offset;
> -	}
> -	ins(BPF_LDX_MEM(BPF_DW, target_reg, ctx_reg, offset), pos);
> -
> -	return check_pos(pos);
> -}
> -
> -/*
> - * Generate a BPF_FUNC_probe_read function call.
> - *
> - * src_base_addr_reg is a register holding base address,
> - * dst_addr_reg is a register holding dest address (on stack),
> - * result is:
> - *
> - *  *[dst_addr_reg] = *([src_base_addr_reg] + offset)
> - *
> - * Arguments of BPF_FUNC_probe_read:
> - *     ARG1: ptr to stack (dest)
> - *     ARG2: size (8)
> - *     ARG3: unsafe ptr (src)
> - */
> -static int
> -gen_read_mem(struct bpf_insn_pos *pos,
> -	     int src_base_addr_reg,
> -	     int dst_addr_reg,
> -	     long offset,
> -	     int probeid)
> -{
> -	/* mov arg3, src_base_addr_reg */
> -	if (src_base_addr_reg != BPF_REG_ARG3)
> -		ins(BPF_MOV64_REG(BPF_REG_ARG3, src_base_addr_reg), pos);
> -	/* add arg3, #offset */
> -	if (offset)
> -		ins(BPF_ALU64_IMM(BPF_ADD, BPF_REG_ARG3, offset), pos);
> -
> -	/* mov arg2, #reg_size */
> -	ins(BPF_ALU64_IMM(BPF_MOV, BPF_REG_ARG2, BPF_REG_SIZE), pos);
> -
> -	/* mov arg1, dst_addr_reg */
> -	if (dst_addr_reg != BPF_REG_ARG1)
> -		ins(BPF_MOV64_REG(BPF_REG_ARG1, dst_addr_reg), pos);
> -
> -	/* Call probe_read  */
> -	ins(BPF_EMIT_CALL(probeid), pos);
> -	/*
> -	 * Error processing: if read fail, goto error code,
> -	 * will be relocated. Target should be the start of
> -	 * error processing code.
> -	 */
> -	ins(BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, JMP_TO_ERROR_CODE),
> -	    pos);
> -
> -	return check_pos(pos);
> -}
> -
> -/*
> - * Each arg should be bare register. Fetch and save them into argument
> - * registers (r3 - r5).
> - *
> - * BPF_REG_1 should have been initialized with pointer to
> - * 'struct pt_regs'.
> - */
> -static int
> -gen_prologue_fastpath(struct bpf_insn_pos *pos,
> -		      struct probe_trace_arg *args, int nargs)
> -{
> -	int i, err = 0;
> -
> -	for (i = 0; i < nargs; i++) {
> -		err = gen_ldx_reg_from_ctx(pos, BPF_REG_1, args[i].value,
> -					   BPF_PROLOGUE_START_ARG_REG + i);
> -		if (err)
> -			goto errout;
> -	}
> -
> -	return check_pos(pos);
> -errout:
> -	return err;
> -}
> -
> -/*
> - * Slow path:
> - *   At least one argument has the form of 'offset($rx)'.
> - *
> - * Following code first stores them into stack, then loads all of then
> - * to r2 - r5.
> - * Before final loading, the final result should be:
> - *
> - * low address
> - * BPF_REG_FP - 24  ARG3
> - * BPF_REG_FP - 16  ARG2
> - * BPF_REG_FP - 8   ARG1
> - * BPF_REG_FP
> - * high address
> - *
> - * For each argument (described as: offn(...off2(off1(reg)))),
> - * generates following code:
> - *
> - *  r7 <- fp
> - *  r7 <- r7 - stack_offset  // Ideal code should initialize r7 using
> - *                           // fp before generating args. However,
> - *                           // eBPF won't regard r7 as stack pointer
> - *                           // if it is generated by minus 8 from
> - *                           // another stack pointer except fp.
> - *                           // This is why we have to set r7
> - *                           // to fp for each variable.
> - *  r3 <- value of 'reg'-> generated using gen_ldx_reg_from_ctx()
> - *  (r7) <- r3       // skip following instructions for bare reg
> - *  r3 <- r3 + off1  . // skip if off1 == 0
> - *  r2 <- 8           \
> - *  r1 <- r7           |-> generated by gen_read_mem()
> - *  call probe_read    /
> - *  jnei r0, 0, err  ./
> - *  r3 <- (r7)
> - *  r3 <- r3 + off2  . // skip if off2 == 0
> - *  r2 <- 8           \  // r2 may be broken by probe_read, so set again
> - *  r1 <- r7           |-> generated by gen_read_mem()
> - *  call probe_read    /
> - *  jnei r0, 0, err  ./
> - *  ...
> - */
> -static int
> -gen_prologue_slowpath(struct bpf_insn_pos *pos,
> -		      struct probe_trace_arg *args, int nargs)
> -{
> -	int err, i, probeid;
> -
> -	for (i = 0; i < nargs; i++) {
> -		struct probe_trace_arg *arg = &args[i];
> -		const char *reg = arg->value;
> -		struct probe_trace_arg_ref *ref = NULL;
> -		int stack_offset = (i + 1) * -8;
> -
> -		pr_debug("prologue: fetch arg %d, base reg is %s\n",
> -			 i, reg);
> -
> -		/* value of base register is stored into ARG3 */
> -		err = gen_ldx_reg_from_ctx(pos, BPF_REG_CTX, reg,
> -					   BPF_REG_ARG3);
> -		if (err) {
> -			pr_err("prologue: failed to get offset of register %s\n",
> -			       reg);
> -			goto errout;
> -		}
> -
> -		/* Make r7 the stack pointer. */
> -		ins(BPF_MOV64_REG(BPF_REG_7, BPF_REG_FP), pos);
> -		/* r7 += -8 */
> -		ins(BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, stack_offset), pos);
> -		/*
> -		 * Store r3 (base register) onto stack
> -		 * Ensure fp[offset] is set.
> -		 * fp is the only valid base register when storing
> -		 * into stack. We are not allowed to use r7 as base
> -		 * register here.
> -		 */
> -		ins(BPF_STX_MEM(BPF_DW, BPF_REG_FP, BPF_REG_ARG3,
> -				stack_offset), pos);
> -
> -		ref = arg->ref;
> -		probeid = BPF_FUNC_probe_read_kernel;
> -		while (ref) {
> -			pr_debug("prologue: arg %d: offset %ld\n",
> -				 i, ref->offset);
> -
> -			if (ref->user_access)
> -				probeid = BPF_FUNC_probe_read_user;
> -
> -			err = gen_read_mem(pos, BPF_REG_3, BPF_REG_7,
> -					   ref->offset, probeid);
> -			if (err) {
> -				pr_err("prologue: failed to generate probe_read function call\n");
> -				goto errout;
> -			}
> -
> -			ref = ref->next;
> -			/*
> -			 * Load previous result into ARG3. Use
> -			 * BPF_REG_FP instead of r7 because verifier
> -			 * allows FP based addressing only.
> -			 */
> -			if (ref)
> -				ins(BPF_LDX_MEM(BPF_DW, BPF_REG_ARG3,
> -						BPF_REG_FP, stack_offset), pos);
> -		}
> -	}
> -
> -	/* Final pass: read to registers */
> -	for (i = 0; i < nargs; i++) {
> -		int insn_sz = (args[i].ref) ? argtype_to_ldx_size(args[i].type) : BPF_DW;
> -
> -		pr_debug("prologue: load arg %d, insn_sz is %s\n",
> -			 i, insn_sz_to_str(insn_sz));
> -		ins(BPF_LDX_MEM(insn_sz, BPF_PROLOGUE_START_ARG_REG + i,
> -				BPF_REG_FP, -BPF_REG_SIZE * (i + 1)), pos);
> -	}
> -
> -	ins(BPF_JMP_IMM(BPF_JA, BPF_REG_0, 0, JMP_TO_SUCCESS_CODE), pos);
> -
> -	return check_pos(pos);
> -errout:
> -	return err;
> -}
> -
> -static int
> -prologue_relocate(struct bpf_insn_pos *pos, struct bpf_insn *error_code,
> -		  struct bpf_insn *success_code, struct bpf_insn *user_code)
> -{
> -	struct bpf_insn *insn;
> -
> -	if (check_pos(pos))
> -		return -BPF_LOADER_ERRNO__PROLOGUE2BIG;
> -
> -	for (insn = pos->begin; insn < pos->pos; insn++) {
> -		struct bpf_insn *target;
> -		u8 class = BPF_CLASS(insn->code);
> -		u8 opcode;
> -
> -		if (class != BPF_JMP)
> -			continue;
> -		opcode = BPF_OP(insn->code);
> -		if (opcode == BPF_CALL)
> -			continue;
> -
> -		switch (insn->off) {
> -		case JMP_TO_ERROR_CODE:
> -			target = error_code;
> -			break;
> -		case JMP_TO_SUCCESS_CODE:
> -			target = success_code;
> -			break;
> -		case JMP_TO_USER_CODE:
> -			target = user_code;
> -			break;
> -		default:
> -			pr_err("bpf prologue: internal error: relocation failed\n");
> -			return -BPF_LOADER_ERRNO__PROLOGUE;
> -		}
> -
> -		insn->off = target - (insn + 1);
> -	}
> -	return 0;
> -}
> -
> -int bpf__gen_prologue(struct probe_trace_arg *args, int nargs,
> -		      struct bpf_insn *new_prog, size_t *new_cnt,
> -		      size_t cnt_space)
> -{
> -	struct bpf_insn *success_code = NULL;
> -	struct bpf_insn *error_code = NULL;
> -	struct bpf_insn *user_code = NULL;
> -	struct bpf_insn_pos pos;
> -	bool fastpath = true;
> -	int err = 0, i;
> -
> -	if (!new_prog || !new_cnt)
> -		return -EINVAL;
> -
> -	if (cnt_space > BPF_MAXINSNS)
> -		cnt_space = BPF_MAXINSNS;
> -
> -	pos.begin = new_prog;
> -	pos.end = new_prog + cnt_space;
> -	pos.pos = new_prog;
> -
> -	if (!nargs) {
> -		ins(BPF_ALU64_IMM(BPF_MOV, BPF_PROLOGUE_FETCH_RESULT_REG, 0),
> -		    &pos);
> -
> -		if (check_pos(&pos))
> -			goto errout;
> -
> -		*new_cnt = pos_get_cnt(&pos);
> -		return 0;
> -	}
> -
> -	if (nargs > BPF_PROLOGUE_MAX_ARGS) {
> -		pr_warning("bpf: prologue: %d arguments are dropped\n",
> -			   nargs - BPF_PROLOGUE_MAX_ARGS);
> -		nargs = BPF_PROLOGUE_MAX_ARGS;
> -	}
> -
> -	/* First pass: validation */
> -	for (i = 0; i < nargs; i++) {
> -		struct probe_trace_arg_ref *ref = args[i].ref;
> -
> -		if (args[i].value[0] == '@') {
> -			/* TODO: fetch global variable */
> -			pr_err("bpf: prologue: global %s%+ld not support\n",
> -				args[i].value, ref ? ref->offset : 0);
> -			return -ENOTSUP;
> -		}
> -
> -		while (ref) {
> -			/* fastpath is true if all args has ref == NULL */
> -			fastpath = false;
> -
> -			/*
> -			 * Instruction encodes immediate value using
> -			 * s32, ref->offset is long. On systems which
> -			 * can't fill long in s32, refuse to process if
> -			 * ref->offset too large (or small).
> -			 */
> -#ifdef __LP64__
> -#define OFFSET_MAX	((1LL << 31) - 1)
> -#define OFFSET_MIN	((1LL << 31) * -1)
> -			if (ref->offset > OFFSET_MAX ||
> -					ref->offset < OFFSET_MIN) {
> -				pr_err("bpf: prologue: offset out of bound: %ld\n",
> -				       ref->offset);
> -				return -BPF_LOADER_ERRNO__PROLOGUEOOB;
> -			}
> -#endif
> -			ref = ref->next;
> -		}
> -	}
> -	pr_debug("prologue: pass validation\n");
> -
> -	if (fastpath) {
> -		/* If all variables are registers... */
> -		pr_debug("prologue: fast path\n");
> -		err = gen_prologue_fastpath(&pos, args, nargs);
> -		if (err)
> -			goto errout;
> -	} else {
> -		pr_debug("prologue: slow path\n");
> -
> -		/* Initialization: move ctx to a callee saved register. */
> -		ins(BPF_MOV64_REG(BPF_REG_CTX, BPF_REG_ARG1), &pos);
> -
> -		err = gen_prologue_slowpath(&pos, args, nargs);
> -		if (err)
> -			goto errout;
> -		/*
> -		 * start of ERROR_CODE (only slow pass needs error code)
> -		 *   mov r2 <- 1  // r2 is error number
> -		 *   mov r3 <- 0  // r3, r4... should be touched or
> -		 *                // verifier would complain
> -		 *   mov r4 <- 0
> -		 *   ...
> -		 *   goto usercode
> -		 */
> -		error_code = pos.pos;
> -		ins(BPF_ALU64_IMM(BPF_MOV, BPF_PROLOGUE_FETCH_RESULT_REG, 1),
> -		    &pos);
> -
> -		for (i = 0; i < nargs; i++)
> -			ins(BPF_ALU64_IMM(BPF_MOV,
> -					  BPF_PROLOGUE_START_ARG_REG + i,
> -					  0),
> -			    &pos);
> -		ins(BPF_JMP_IMM(BPF_JA, BPF_REG_0, 0, JMP_TO_USER_CODE),
> -				&pos);
> -	}
> -
> -	/*
> -	 * start of SUCCESS_CODE:
> -	 *   mov r2 <- 0
> -	 *   goto usercode  // skip
> -	 */
> -	success_code = pos.pos;
> -	ins(BPF_ALU64_IMM(BPF_MOV, BPF_PROLOGUE_FETCH_RESULT_REG, 0), &pos);
> -
> -	/*
> -	 * start of USER_CODE:
> -	 *   Restore ctx to r1
> -	 */
> -	user_code = pos.pos;
> -	if (!fastpath) {
> -		/*
> -		 * Only slow path needs restoring of ctx. In fast path,
> -		 * register are loaded directly from r1.
> -		 */
> -		ins(BPF_MOV64_REG(BPF_REG_ARG1, BPF_REG_CTX), &pos);
> -		err = prologue_relocate(&pos, error_code, success_code,
> -					user_code);
> -		if (err)
> -			goto errout;
> -	}
> -
> -	err = check_pos(&pos);
> -	if (err)
> -		goto errout;
> -
> -	*new_cnt = pos_get_cnt(&pos);
> -	return 0;
> -errout:
> -	return err;
> -}

-- 

- Arnaldo

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

end of thread, other threads:[~2022-01-22 20:32 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-16 22:21 [PATCH bpf-next 0/2] perf: stop using deprecated bpf APIs Christy Lee
2021-12-16 22:21 ` [PATCH bpf-next 1/2] perf: stop using deprecated bpf_prog_load() API Christy Lee
2021-12-16 22:21 ` [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API Christy Lee
2021-12-21  8:22   ` Jiri Olsa
2021-12-21 21:58     ` Andrii Nakryiko
2021-12-22 13:44       ` Jiri Olsa
2021-12-22 22:17         ` Andrii Nakryiko
2021-12-29 19:01           ` Christy Lee
2022-01-04 14:40             ` Jiri Olsa
2022-01-05 13:49               ` Jiri Olsa
2022-01-06 17:54                 ` Christy Lee
2022-01-06 22:41                   ` Jiri Olsa
2022-01-13 15:14                   ` Jiri Olsa
2022-01-14 21:00                     ` Andrii Nakryiko
2022-01-17  9:25                       ` Jiri Olsa
2022-01-18 23:05                         ` Andrii Nakryiko
2022-01-22 20:29                         ` Arnaldo Carvalho de Melo
2022-01-06 20:25                 ` Arnaldo Carvalho de Melo

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