[PATCH perf/core 0/5] perf tools: Fix prologue generation

[PATCH perf/core 0/5] perf tools: Fix prologue generation

[Jiri Olsa]

hi,
sending change we discussed some time ago [1] to get rid of
some deprecated functions we use in perf prologue code.

Despite the gloomy discussion I think the final code does
not look that bad ;-)

This patchset removes following libbpf functions from perf:
  bpf_program__set_prep
  bpf_program__nth_fd
  struct bpf_prog_prep_result

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

thanks,
jirka


[1] https://lore.kernel.org/bpf/CAEf4BzaiBO3_617kkXZdYJ8hS8YF--ZLgapNbgeeEJ-pY0H88g@mail.gmail.com/
---
Jiri Olsa (5):
      libbpf: Add bpf_program__set_insns function
      libbpf: Load prog's instructions after prog_prepare_load_fn callback
      perf tools: Move libbpf init in libbpf_init function
      perf tools: Register perfkprobe libbpf section handler
      perf tools: Rework prologue generation code

 tools/lib/bpf/libbpf.c                      |  10 +++++
 tools/lib/bpf/libbpf.h                      |  12 ++++++
 tools/lib/bpf/libbpf.map                    |   3 +-
 tools/perf/include/bpf/bpf.h                |   2 +-
 tools/perf/tests/bpf-script-example.c       |   2 +-
 tools/perf/tests/bpf-script-test-prologue.c |   2 +-
 tools/perf/util/bpf-loader.c                | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------
 7 files changed, 212 insertions(+), 32 deletions(-)

[PATCH perf/core 1/5] libbpf: Add bpf_program__set_insns function

[Jiri Olsa]

Adding bpf_program__set_insns that allows to set new
instructions for program.

Also moving bpf_program__attach_kprobe_multi_opts on
the proper name sorted place in map file.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/libbpf.c   |  8 ++++++++
 tools/lib/bpf/libbpf.h   | 12 ++++++++++++
 tools/lib/bpf/libbpf.map |  3 ++-
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 809fe209cdcc..284790d81c1b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -8457,6 +8457,14 @@ size_t bpf_program__insn_cnt(const struct bpf_program *prog)
 	return prog->insns_cnt;
 }
 
+void bpf_program__set_insns(struct bpf_program *prog,
+			    struct bpf_insn *insns, size_t insns_cnt)
+{
+	free(prog->insns);
+	prog->insns = insns;
+	prog->insns_cnt = insns_cnt;
+}
+
 int bpf_program__set_prep(struct bpf_program *prog, int nr_instances,
 			  bpf_program_prep_t prep)
 {
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 05dde85e19a6..b31ad58d335f 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -323,6 +323,18 @@ struct bpf_insn;
  * different.
  */
 LIBBPF_API const struct bpf_insn *bpf_program__insns(const struct bpf_program *prog);
+
+/**
+ * @brief **bpf_program__set_insns()** can set BPF program's underlying
+ * BPF instructions.
+ * @param prog BPF program for which to return instructions
+ * @param insn a pointer to an array of BPF instructions
+ * @param insns_cnt number of `struct bpf_insn`'s that form
+ * specified BPF program
+ */
+LIBBPF_API void bpf_program__set_insns(struct bpf_program *prog,
+				       struct bpf_insn *insns, size_t insns_cnt);
+
 /**
  * @brief **bpf_program__insn_cnt()** returns number of `struct bpf_insn`'s
  * that form specified BPF program.
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index dd35ee58bfaa..afa10d24ab41 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -444,7 +444,8 @@ LIBBPF_0.8.0 {
 	global:
 		bpf_object__destroy_subskeleton;
 		bpf_object__open_subskeleton;
+		bpf_program__attach_kprobe_multi_opts;
+		bpf_program__set_insns;
 		libbpf_register_prog_handler;
 		libbpf_unregister_prog_handler;
-		bpf_program__attach_kprobe_multi_opts;
 } LIBBPF_0.7.0;
-- 
2.35.1

Re: [PATCH perf/core 1/5] libbpf: Add bpf_program__set_insns function

[Daniel Borkmann]

On 4/22/22 12:00 PM, Jiri Olsa wrote:
> Adding bpf_program__set_insns that allows to set new
> instructions for program.
> 
> Also moving bpf_program__attach_kprobe_multi_opts on
> the proper name sorted place in map file.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>   tools/lib/bpf/libbpf.c   |  8 ++++++++
>   tools/lib/bpf/libbpf.h   | 12 ++++++++++++
>   tools/lib/bpf/libbpf.map |  3 ++-
>   3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 809fe209cdcc..284790d81c1b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -8457,6 +8457,14 @@ size_t bpf_program__insn_cnt(const struct bpf_program *prog)
>   	return prog->insns_cnt;
>   }
>   
> +void bpf_program__set_insns(struct bpf_program *prog,
> +			    struct bpf_insn *insns, size_t insns_cnt)
> +{
> +	free(prog->insns);
> +	prog->insns = insns;
> +	prog->insns_cnt = insns_cnt;
> +}
> +
>   int bpf_program__set_prep(struct bpf_program *prog, int nr_instances,
>   			  bpf_program_prep_t prep)
>   {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 05dde85e19a6..b31ad58d335f 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -323,6 +323,18 @@ struct bpf_insn;
>    * different.
>    */
>   LIBBPF_API const struct bpf_insn *bpf_program__insns(const struct bpf_program *prog);
> +
> +/**
> + * @brief **bpf_program__set_insns()** can set BPF program's underlying
> + * BPF instructions.
> + * @param prog BPF program for which to return instructions
> + * @param insn a pointer to an array of BPF instructions
> + * @param insns_cnt number of `struct bpf_insn`'s that form
> + * specified BPF program
> + */
> +LIBBPF_API void bpf_program__set_insns(struct bpf_program *prog,
> +				       struct bpf_insn *insns, size_t insns_cnt);
> +

Iiuc, patch 2 should be squashed into this one given they logically belong to the
same change?

Fwiw, I think the API description should be elaborated a bit more, in particular that
the passed-in insns need to be from allocated dynamic memory which is later on passed
to free(), and maybe also constraints at which point in time bpf_program__set_insns()
may be called.. (as well as high-level description on potential use cases e.g. around
patch 4).

>   /**
>    * @brief **bpf_program__insn_cnt()** returns number of `struct bpf_insn`'s
>    * that form specified BPF program.
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index dd35ee58bfaa..afa10d24ab41 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -444,7 +444,8 @@ LIBBPF_0.8.0 {
>   	global:
>   		bpf_object__destroy_subskeleton;
>   		bpf_object__open_subskeleton;
> +		bpf_program__attach_kprobe_multi_opts;
> +		bpf_program__set_insns;
>   		libbpf_register_prog_handler;
>   		libbpf_unregister_prog_handler;
> -		bpf_program__attach_kprobe_multi_opts;
>   } LIBBPF_0.7.0;
> 

Re: [PATCH perf/core 1/5] libbpf: Add bpf_program__set_insns function

[Jiri Olsa]

On Mon, Apr 25, 2022 at 06:22:37PM +0200, Daniel Borkmann wrote:
> On 4/22/22 12:00 PM, Jiri Olsa wrote:
> > Adding bpf_program__set_insns that allows to set new
> > instructions for program.
> > 
> > Also moving bpf_program__attach_kprobe_multi_opts on
> > the proper name sorted place in map file.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   tools/lib/bpf/libbpf.c   |  8 ++++++++
> >   tools/lib/bpf/libbpf.h   | 12 ++++++++++++
> >   tools/lib/bpf/libbpf.map |  3 ++-
> >   3 files changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 809fe209cdcc..284790d81c1b 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -8457,6 +8457,14 @@ size_t bpf_program__insn_cnt(const struct bpf_program *prog)
> >   	return prog->insns_cnt;
> >   }
> > +void bpf_program__set_insns(struct bpf_program *prog,
> > +			    struct bpf_insn *insns, size_t insns_cnt)
> > +{
> > +	free(prog->insns);
> > +	prog->insns = insns;
> > +	prog->insns_cnt = insns_cnt;
> > +}
> > +
> >   int bpf_program__set_prep(struct bpf_program *prog, int nr_instances,
> >   			  bpf_program_prep_t prep)
> >   {
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 05dde85e19a6..b31ad58d335f 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -323,6 +323,18 @@ struct bpf_insn;
> >    * different.
> >    */
> >   LIBBPF_API const struct bpf_insn *bpf_program__insns(const struct bpf_program *prog);
> > +
> > +/**
> > + * @brief **bpf_program__set_insns()** can set BPF program's underlying
> > + * BPF instructions.
> > + * @param prog BPF program for which to return instructions
> > + * @param insn a pointer to an array of BPF instructions
> > + * @param insns_cnt number of `struct bpf_insn`'s that form
> > + * specified BPF program
> > + */
> > +LIBBPF_API void bpf_program__set_insns(struct bpf_program *prog,
> > +				       struct bpf_insn *insns, size_t insns_cnt);
> > +
> 
> Iiuc, patch 2 should be squashed into this one given they logically belong to the
> same change?

right, that's probably better

> 
> Fwiw, I think the API description should be elaborated a bit more, in particular that
> the passed-in insns need to be from allocated dynamic memory which is later on passed
> to free(), and maybe also constraints at which point in time bpf_program__set_insns()
> may be called.. (as well as high-level description on potential use cases e.g. around
> patch 4).

ok, will add that

thanks,
jirka

> 
> >   /**
> >    * @brief **bpf_program__insn_cnt()** returns number of `struct bpf_insn`'s
> >    * that form specified BPF program.
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index dd35ee58bfaa..afa10d24ab41 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -444,7 +444,8 @@ LIBBPF_0.8.0 {
> >   	global:
> >   		bpf_object__destroy_subskeleton;
> >   		bpf_object__open_subskeleton;
> > +		bpf_program__attach_kprobe_multi_opts;
> > +		bpf_program__set_insns;
> >   		libbpf_register_prog_handler;
> >   		libbpf_unregister_prog_handler;
> > -		bpf_program__attach_kprobe_multi_opts;
> >   } LIBBPF_0.7.0;
> > 
> 

Re: [PATCH perf/core 1/5] libbpf: Add bpf_program__set_insns function

[Andrii Nakryiko]

On Mon, Apr 25, 2022 at 9:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 4/22/22 12:00 PM, Jiri Olsa wrote:
> > Adding bpf_program__set_insns that allows to set new
> > instructions for program.
> >
> > Also moving bpf_program__attach_kprobe_multi_opts on
> > the proper name sorted place in map file.

would make sense to fix it as a separate patch, it has nothing to do
with bpf_program__set_insns() API itself

> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   tools/lib/bpf/libbpf.c   |  8 ++++++++
> >   tools/lib/bpf/libbpf.h   | 12 ++++++++++++
> >   tools/lib/bpf/libbpf.map |  3 ++-
> >   3 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 809fe209cdcc..284790d81c1b 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -8457,6 +8457,14 @@ size_t bpf_program__insn_cnt(const struct bpf_program *prog)
> >       return prog->insns_cnt;
> >   }
> >
> > +void bpf_program__set_insns(struct bpf_program *prog,
> > +                         struct bpf_insn *insns, size_t insns_cnt)
> > +{
> > +     free(prog->insns);
> > +     prog->insns = insns;
> > +     prog->insns_cnt = insns_cnt;

let's not store user-provided pointer here. Please realloc prog->insns
as necessary and copy over insns into it.

Also let's at least add the check for prog->loaded and return -EBUSY
in such a case. And of course this API should return int, not void.

> > +}
> > +
> >   int bpf_program__set_prep(struct bpf_program *prog, int nr_instances,
> >                         bpf_program_prep_t prep)
> >   {
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 05dde85e19a6..b31ad58d335f 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -323,6 +323,18 @@ struct bpf_insn;
> >    * different.
> >    */
> >   LIBBPF_API const struct bpf_insn *bpf_program__insns(const struct bpf_program *prog);
> > +
> > +/**
> > + * @brief **bpf_program__set_insns()** can set BPF program's underlying
> > + * BPF instructions.
> > + * @param prog BPF program for which to return instructions
> > + * @param insn a pointer to an array of BPF instructions
> > + * @param insns_cnt number of `struct bpf_insn`'s that form
> > + * specified BPF program
> > + */

This API makes me want to cry... but I can't come up with anything
better for perf's use case.

But it can only more or less safely and sanely be used from the
prog_prepare_load_fn callback, so please add a big warning here saying
that this is a very advanced libbpf API and the user needs to know
what they are doing and this should be used from prog_prepare_load_fn
callback only. If bpf_program__set_insns() is called before
prog_prepare_load_fn any map/subprog/etc relocation will most probably
fail or corrupt BPF program code.

> > +LIBBPF_API void bpf_program__set_insns(struct bpf_program *prog,
> > +                                    struct bpf_insn *insns, size_t insns_cnt);

s/insns_cnt/insn_cnt/

> > +
>
> Iiuc, patch 2 should be squashed into this one given they logically belong to the
> same change?
>
> Fwiw, I think the API description should be elaborated a bit more, in particular that
> the passed-in insns need to be from allocated dynamic memory which is later on passed
> to free(), and maybe also constraints at which point in time bpf_program__set_insns()
> may be called.. (as well as high-level description on potential use cases e.g. around
> patch 4).

Yep, patch #1 is kind of broken without patch #2, so let's combine them.

>
> >   /**
> >    * @brief **bpf_program__insn_cnt()** returns number of `struct bpf_insn`'s
> >    * that form specified BPF program.
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index dd35ee58bfaa..afa10d24ab41 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -444,7 +444,8 @@ LIBBPF_0.8.0 {
> >       global:
> >               bpf_object__destroy_subskeleton;
> >               bpf_object__open_subskeleton;
> > +             bpf_program__attach_kprobe_multi_opts;
> > +             bpf_program__set_insns;
> >               libbpf_register_prog_handler;
> >               libbpf_unregister_prog_handler;
> > -             bpf_program__attach_kprobe_multi_opts;
> >   } LIBBPF_0.7.0;
> >
>

Re: [PATCH perf/core 1/5] libbpf: Add bpf_program__set_insns function

[Jiri Olsa]

On Mon, Apr 25, 2022 at 11:19:09PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 25, 2022 at 9:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 4/22/22 12:00 PM, Jiri Olsa wrote:
> > > Adding bpf_program__set_insns that allows to set new
> > > instructions for program.
> > >
> > > Also moving bpf_program__attach_kprobe_multi_opts on
> > > the proper name sorted place in map file.
> 
> would make sense to fix it as a separate patch, it has nothing to do
> with bpf_program__set_insns() API itself

np

> 
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >   tools/lib/bpf/libbpf.c   |  8 ++++++++
> > >   tools/lib/bpf/libbpf.h   | 12 ++++++++++++
> > >   tools/lib/bpf/libbpf.map |  3 ++-
> > >   3 files changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 809fe209cdcc..284790d81c1b 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -8457,6 +8457,14 @@ size_t bpf_program__insn_cnt(const struct bpf_program *prog)
> > >       return prog->insns_cnt;
> > >   }
> > >
> > > +void bpf_program__set_insns(struct bpf_program *prog,
> > > +                         struct bpf_insn *insns, size_t insns_cnt)
> > > +{
> > > +     free(prog->insns);
> > > +     prog->insns = insns;
> > > +     prog->insns_cnt = insns_cnt;
> 
> let's not store user-provided pointer here. Please realloc prog->insns
> as necessary and copy over insns into it.
> 
> Also let's at least add the check for prog->loaded and return -EBUSY
> in such a case. And of course this API should return int, not void.

ok, will change

> 
> > > +}
> > > +
> > >   int bpf_program__set_prep(struct bpf_program *prog, int nr_instances,
> > >                         bpf_program_prep_t prep)
> > >   {
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index 05dde85e19a6..b31ad58d335f 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> > > @@ -323,6 +323,18 @@ struct bpf_insn;
> > >    * different.
> > >    */
> > >   LIBBPF_API const struct bpf_insn *bpf_program__insns(const struct bpf_program *prog);
> > > +
> > > +/**
> > > + * @brief **bpf_program__set_insns()** can set BPF program's underlying
> > > + * BPF instructions.
> > > + * @param prog BPF program for which to return instructions
> > > + * @param insn a pointer to an array of BPF instructions
> > > + * @param insns_cnt number of `struct bpf_insn`'s that form
> > > + * specified BPF program
> > > + */
> 
> This API makes me want to cry... but I can't come up with anything
> better for perf's use case.
> 
> But it can only more or less safely and sanely be used from the
> prog_prepare_load_fn callback, so please add a big warning here saying
> that this is a very advanced libbpf API and the user needs to know
> what they are doing and this should be used from prog_prepare_load_fn
> callback only. If bpf_program__set_insns() is called before
> prog_prepare_load_fn any map/subprog/etc relocation will most probably
> fail or corrupt BPF program code.

will add the warnings

> 
> > > +LIBBPF_API void bpf_program__set_insns(struct bpf_program *prog,
> > > +                                    struct bpf_insn *insns, size_t insns_cnt);
> 
> s/insns_cnt/insn_cnt/
> 
> > > +
> >
> > Iiuc, patch 2 should be squashed into this one given they logically belong to the
> > same change?
> >
> > Fwiw, I think the API description should be elaborated a bit more, in particular that
> > the passed-in insns need to be from allocated dynamic memory which is later on passed
> > to free(), and maybe also constraints at which point in time bpf_program__set_insns()
> > may be called.. (as well as high-level description on potential use cases e.g. around
> > patch 4).
> 
> Yep, patch #1 is kind of broken without patch #2, so let's combine them.

ok

thanks,
jirka

Re: [PATCH perf/core 1/5] libbpf: Add bpf_program__set_insns function

[Andrii Nakryiko]

On Mon, Apr 25, 2022 at 11:57 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Mon, Apr 25, 2022 at 11:19:09PM -0700, Andrii Nakryiko wrote:
> > On Mon, Apr 25, 2022 at 9:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > >
> > > On 4/22/22 12:00 PM, Jiri Olsa wrote:
> > > > Adding bpf_program__set_insns that allows to set new
> > > > instructions for program.
> > > >
> > > > Also moving bpf_program__attach_kprobe_multi_opts on
> > > > the proper name sorted place in map file.
> >
> > would make sense to fix it as a separate patch, it has nothing to do
> > with bpf_program__set_insns() API itself
>
> np
>
> >
> > > >
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >   tools/lib/bpf/libbpf.c   |  8 ++++++++
> > > >   tools/lib/bpf/libbpf.h   | 12 ++++++++++++
> > > >   tools/lib/bpf/libbpf.map |  3 ++-
> > > >   3 files changed, 22 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > index 809fe209cdcc..284790d81c1b 100644
> > > > --- a/tools/lib/bpf/libbpf.c
> > > > +++ b/tools/lib/bpf/libbpf.c
> > > > @@ -8457,6 +8457,14 @@ size_t bpf_program__insn_cnt(const struct bpf_program *prog)
> > > >       return prog->insns_cnt;
> > > >   }
> > > >
> > > > +void bpf_program__set_insns(struct bpf_program *prog,
> > > > +                         struct bpf_insn *insns, size_t insns_cnt)
> > > > +{
> > > > +     free(prog->insns);
> > > > +     prog->insns = insns;
> > > > +     prog->insns_cnt = insns_cnt;
> >
> > let's not store user-provided pointer here. Please realloc prog->insns
> > as necessary and copy over insns into it.
> >
> > Also let's at least add the check for prog->loaded and return -EBUSY
> > in such a case. And of course this API should return int, not void.
>
> ok, will change
>
> >
> > > > +}
> > > > +
> > > >   int bpf_program__set_prep(struct bpf_program *prog, int nr_instances,
> > > >                         bpf_program_prep_t prep)
> > > >   {
> > > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > > index 05dde85e19a6..b31ad58d335f 100644
> > > > --- a/tools/lib/bpf/libbpf.h
> > > > +++ b/tools/lib/bpf/libbpf.h
> > > > @@ -323,6 +323,18 @@ struct bpf_insn;
> > > >    * different.
> > > >    */
> > > >   LIBBPF_API const struct bpf_insn *bpf_program__insns(const struct bpf_program *prog);
> > > > +
> > > > +/**
> > > > + * @brief **bpf_program__set_insns()** can set BPF program's underlying
> > > > + * BPF instructions.
> > > > + * @param prog BPF program for which to return instructions
> > > > + * @param insn a pointer to an array of BPF instructions
> > > > + * @param insns_cnt number of `struct bpf_insn`'s that form
> > > > + * specified BPF program
> > > > + */
> >
> > This API makes me want to cry... but I can't come up with anything
> > better for perf's use case.
> >

So thinking about this some more. If we make libbpf not to close maps
and prog FDs on BPF program load failure automatically and instead
doing it in bpf_object__close(), which would seem to be a totally fine
semantics and won't break any reasonable application as they always
have to call bpf_object__close() anyways to clean up all the
resources; we wouldn't need this horror of bpf_program__set_insns().
Your BPF program would fail to load, but you'll get its fully prepared
instructions with bpf_program__insns(), then you can just append
correct preamble. Meanwhile, all the maps will be created (they are
always created before the first program load), so all the FDs will be
correct.

This is certainly advanced knowledge of libbpf behavior, but the use
case you are trying to solve is also very unique and advanced (and I
wouldn't recommend anyone trying to do this anyways). WDYT? Would that
work?

> > But it can only more or less safely and sanely be used from the
> > prog_prepare_load_fn callback, so please add a big warning here saying
> > that this is a very advanced libbpf API and the user needs to know
> > what they are doing and this should be used from prog_prepare_load_fn
> > callback only. If bpf_program__set_insns() is called before
> > prog_prepare_load_fn any map/subprog/etc relocation will most probably
> > fail or corrupt BPF program code.
>
> will add the warnings
>
> >
> > > > +LIBBPF_API void bpf_program__set_insns(struct bpf_program *prog,
> > > > +                                    struct bpf_insn *insns, size_t insns_cnt);
> >
> > s/insns_cnt/insn_cnt/
> >
> > > > +
> > >
> > > Iiuc, patch 2 should be squashed into this one given they logically belong to the
> > > same change?
> > >
> > > Fwiw, I think the API description should be elaborated a bit more, in particular that
> > > the passed-in insns need to be from allocated dynamic memory which is later on passed
> > > to free(), and maybe also constraints at which point in time bpf_program__set_insns()
> > > may be called.. (as well as high-level description on potential use cases e.g. around
> > > patch 4).
> >
> > Yep, patch #1 is kind of broken without patch #2, so let's combine them.
>
> ok
>
> thanks,
> jirka

Re: [PATCH perf/core 1/5] libbpf: Add bpf_program__set_insns function

[Jiri Olsa]

On Tue, Apr 26, 2022 at 08:58:12AM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 25, 2022 at 11:57 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Mon, Apr 25, 2022 at 11:19:09PM -0700, Andrii Nakryiko wrote:
> > > On Mon, Apr 25, 2022 at 9:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > >
> > > > On 4/22/22 12:00 PM, Jiri Olsa wrote:
> > > > > Adding bpf_program__set_insns that allows to set new
> > > > > instructions for program.
> > > > >
> > > > > Also moving bpf_program__attach_kprobe_multi_opts on
> > > > > the proper name sorted place in map file.
> > >
> > > would make sense to fix it as a separate patch, it has nothing to do
> > > with bpf_program__set_insns() API itself
> >
> > np
> >
> > >
> > > > >
> > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > ---
> > > > >   tools/lib/bpf/libbpf.c   |  8 ++++++++
> > > > >   tools/lib/bpf/libbpf.h   | 12 ++++++++++++
> > > > >   tools/lib/bpf/libbpf.map |  3 ++-
> > > > >   3 files changed, 22 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > > index 809fe209cdcc..284790d81c1b 100644
> > > > > --- a/tools/lib/bpf/libbpf.c
> > > > > +++ b/tools/lib/bpf/libbpf.c
> > > > > @@ -8457,6 +8457,14 @@ size_t bpf_program__insn_cnt(const struct bpf_program *prog)
> > > > >       return prog->insns_cnt;
> > > > >   }
> > > > >
> > > > > +void bpf_program__set_insns(struct bpf_program *prog,
> > > > > +                         struct bpf_insn *insns, size_t insns_cnt)
> > > > > +{
> > > > > +     free(prog->insns);
> > > > > +     prog->insns = insns;
> > > > > +     prog->insns_cnt = insns_cnt;
> > >
> > > let's not store user-provided pointer here. Please realloc prog->insns
> > > as necessary and copy over insns into it.
> > >
> > > Also let's at least add the check for prog->loaded and return -EBUSY
> > > in such a case. And of course this API should return int, not void.
> >
> > ok, will change
> >
> > >
> > > > > +}
> > > > > +
> > > > >   int bpf_program__set_prep(struct bpf_program *prog, int nr_instances,
> > > > >                         bpf_program_prep_t prep)
> > > > >   {
> > > > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > > > index 05dde85e19a6..b31ad58d335f 100644
> > > > > --- a/tools/lib/bpf/libbpf.h
> > > > > +++ b/tools/lib/bpf/libbpf.h
> > > > > @@ -323,6 +323,18 @@ struct bpf_insn;
> > > > >    * different.
> > > > >    */
> > > > >   LIBBPF_API const struct bpf_insn *bpf_program__insns(const struct bpf_program *prog);
> > > > > +
> > > > > +/**
> > > > > + * @brief **bpf_program__set_insns()** can set BPF program's underlying
> > > > > + * BPF instructions.
> > > > > + * @param prog BPF program for which to return instructions
> > > > > + * @param insn a pointer to an array of BPF instructions
> > > > > + * @param insns_cnt number of `struct bpf_insn`'s that form
> > > > > + * specified BPF program
> > > > > + */
> > >
> > > This API makes me want to cry... but I can't come up with anything
> > > better for perf's use case.
> > >
> 
> So thinking about this some more. If we make libbpf not to close maps
> and prog FDs on BPF program load failure automatically and instead
> doing it in bpf_object__close(), which would seem to be a totally fine
> semantics and won't break any reasonable application as they always
> have to call bpf_object__close() anyways to clean up all the
> resources; we wouldn't need this horror of bpf_program__set_insns().
> Your BPF program would fail to load, but you'll get its fully prepared
> instructions with bpf_program__insns(), then you can just append
> correct preamble. Meanwhile, all the maps will be created (they are
> always created before the first program load), so all the FDs will be
> correct.
> 
> This is certainly advanced knowledge of libbpf behavior, but the use
> case you are trying to solve is also very unique and advanced (and I
> wouldn't recommend anyone trying to do this anyways). WDYT? Would that
> work?

hm, so verifier will fail after all maps are set up during the walk
of the program instructions.. I guess that could work, I'll give it
a try, should be easy change in libbpf (like below) and also in perf

but still the bpf_program__set_insns seems less horror to me

jirka


---
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index c8df74e5f658..1eb75d4231ff 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -7577,19 +7577,6 @@ static int bpf_object_load(struct bpf_object *obj, int extra_log_level, const ch
 	obj->btf_vmlinux = NULL;
 
 	obj->loaded = true; /* doesn't matter if successfully or not */
-
-	if (err)
-		goto out;
-
-	return 0;
-out:
-	/* unpin any maps that were auto-pinned during load */
-	for (i = 0; i < obj->nr_maps; i++)
-		if (obj->maps[i].pinned && !obj->maps[i].reused)
-			bpf_map__unpin(&obj->maps[i], NULL);
-
-	bpf_object_unload(obj);
-	pr_warn("failed to load object '%s'\n", obj->path);
 	return libbpf_err(err);
 }
 

Re: [PATCH perf/core 1/5] libbpf: Add bpf_program__set_insns function

[Andrii Nakryiko]

On Wed, Apr 27, 2022 at 1:42 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Apr 26, 2022 at 08:58:12AM -0700, Andrii Nakryiko wrote:
> > On Mon, Apr 25, 2022 at 11:57 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Mon, Apr 25, 2022 at 11:19:09PM -0700, Andrii Nakryiko wrote:
> > > > On Mon, Apr 25, 2022 at 9:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > > >
> > > > > On 4/22/22 12:00 PM, Jiri Olsa wrote:
> > > > > > Adding bpf_program__set_insns that allows to set new
> > > > > > instructions for program.
> > > > > >
> > > > > > Also moving bpf_program__attach_kprobe_multi_opts on
> > > > > > the proper name sorted place in map file.
> > > >
> > > > would make sense to fix it as a separate patch, it has nothing to do
> > > > with bpf_program__set_insns() API itself
> > >
> > > np
> > >
> > > >
> > > > > >
> > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > > ---
> > > > > >   tools/lib/bpf/libbpf.c   |  8 ++++++++
> > > > > >   tools/lib/bpf/libbpf.h   | 12 ++++++++++++
> > > > > >   tools/lib/bpf/libbpf.map |  3 ++-
> > > > > >   3 files changed, 22 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > > > index 809fe209cdcc..284790d81c1b 100644
> > > > > > --- a/tools/lib/bpf/libbpf.c
> > > > > > +++ b/tools/lib/bpf/libbpf.c
> > > > > > @@ -8457,6 +8457,14 @@ size_t bpf_program__insn_cnt(const struct bpf_program *prog)
> > > > > >       return prog->insns_cnt;
> > > > > >   }
> > > > > >
> > > > > > +void bpf_program__set_insns(struct bpf_program *prog,
> > > > > > +                         struct bpf_insn *insns, size_t insns_cnt)
> > > > > > +{
> > > > > > +     free(prog->insns);
> > > > > > +     prog->insns = insns;
> > > > > > +     prog->insns_cnt = insns_cnt;
> > > >
> > > > let's not store user-provided pointer here. Please realloc prog->insns
> > > > as necessary and copy over insns into it.
> > > >
> > > > Also let's at least add the check for prog->loaded and return -EBUSY
> > > > in such a case. And of course this API should return int, not void.
> > >
> > > ok, will change
> > >
> > > >
> > > > > > +}
> > > > > > +
> > > > > >   int bpf_program__set_prep(struct bpf_program *prog, int nr_instances,
> > > > > >                         bpf_program_prep_t prep)
> > > > > >   {
> > > > > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > > > > index 05dde85e19a6..b31ad58d335f 100644
> > > > > > --- a/tools/lib/bpf/libbpf.h
> > > > > > +++ b/tools/lib/bpf/libbpf.h
> > > > > > @@ -323,6 +323,18 @@ struct bpf_insn;
> > > > > >    * different.
> > > > > >    */
> > > > > >   LIBBPF_API const struct bpf_insn *bpf_program__insns(const struct bpf_program *prog);
> > > > > > +
> > > > > > +/**
> > > > > > + * @brief **bpf_program__set_insns()** can set BPF program's underlying
> > > > > > + * BPF instructions.
> > > > > > + * @param prog BPF program for which to return instructions
> > > > > > + * @param insn a pointer to an array of BPF instructions
> > > > > > + * @param insns_cnt number of `struct bpf_insn`'s that form
> > > > > > + * specified BPF program
> > > > > > + */
> > > >
> > > > This API makes me want to cry... but I can't come up with anything
> > > > better for perf's use case.
> > > >
> >
> > So thinking about this some more. If we make libbpf not to close maps
> > and prog FDs on BPF program load failure automatically and instead
> > doing it in bpf_object__close(), which would seem to be a totally fine
> > semantics and won't break any reasonable application as they always
> > have to call bpf_object__close() anyways to clean up all the
> > resources; we wouldn't need this horror of bpf_program__set_insns().
> > Your BPF program would fail to load, but you'll get its fully prepared
> > instructions with bpf_program__insns(), then you can just append
> > correct preamble. Meanwhile, all the maps will be created (they are
> > always created before the first program load), so all the FDs will be
> > correct.
> >
> > This is certainly advanced knowledge of libbpf behavior, but the use
> > case you are trying to solve is also very unique and advanced (and I
> > wouldn't recommend anyone trying to do this anyways). WDYT? Would that
> > work?
>
> hm, so verifier will fail after all maps are set up during the walk
> of the program instructions.. I guess that could work, I'll give it
> a try, should be easy change in libbpf (like below) and also in perf
>
> but still the bpf_program__set_insns seems less horror to me

let's keep set_insns API then, but please do add a lot of warnings
into the description to make it very-very scary :)

>
> jirka
>
>
> ---
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index c8df74e5f658..1eb75d4231ff 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -7577,19 +7577,6 @@ static int bpf_object_load(struct bpf_object *obj, int extra_log_level, const ch
>         obj->btf_vmlinux = NULL;
>
>         obj->loaded = true; /* doesn't matter if successfully or not */
> -
> -       if (err)
> -               goto out;
> -
> -       return 0;
> -out:
> -       /* unpin any maps that were auto-pinned during load */
> -       for (i = 0; i < obj->nr_maps; i++)
> -               if (obj->maps[i].pinned && !obj->maps[i].reused)
> -                       bpf_map__unpin(&obj->maps[i], NULL);
> -
> -       bpf_object_unload(obj);
> -       pr_warn("failed to load object '%s'\n", obj->path);
>         return libbpf_err(err);
>  }
>

[PATCH perf/core 2/5] libbpf: Load prog's instructions after prog_prepare_load_fn callback

[Jiri Olsa]

We can have changed instructions after calling
prog_prepare_load_fn callback, reload them.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/libbpf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 284790d81c1b..c8df74e5f658 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6695,6 +6695,8 @@ static int bpf_object_load_prog_instance(struct bpf_object *obj, struct bpf_prog
 				prog->name, err);
 			return err;
 		}
+		insns = prog->insns;
+		insns_cnt = prog->insns_cnt;
 	}
 
 	if (obj->gen_loader) {
-- 
2.35.1

[PATCH perf/core 3/5] perf tools: Move libbpf init in libbpf_init function

[Jiri Olsa]

Moving the libbpf init code into single function,
so we have single place doing that.

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

diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index b72cef1ae959..f8ad581ea247 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -99,16 +99,26 @@ static int bpf_perf_object__add(struct bpf_object *obj)
 	return perf_obj ? 0 : -ENOMEM;
 }
 
+static int libbpf_init(void)
+{
+	if (libbpf_initialized)
+		return 0;
+
+	libbpf_set_print(libbpf_perf_print);
+	libbpf_initialized = true;
+	return 0;
+}
+
 struct bpf_object *
 bpf__prepare_load_buffer(void *obj_buf, size_t obj_buf_sz, const char *name)
 {
 	LIBBPF_OPTS(bpf_object_open_opts, opts, .object_name = name);
 	struct bpf_object *obj;
+	int err;
 
-	if (!libbpf_initialized) {
-		libbpf_set_print(libbpf_perf_print);
-		libbpf_initialized = true;
-	}
+	err = libbpf_init();
+	if (err)
+		return ERR_PTR(err);
 
 	obj = bpf_object__open_mem(obj_buf, obj_buf_sz, &opts);
 	if (IS_ERR_OR_NULL(obj)) {
@@ -135,14 +145,13 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
 {
 	LIBBPF_OPTS(bpf_object_open_opts, opts, .object_name = filename);
 	struct bpf_object *obj;
+	int err;
 
-	if (!libbpf_initialized) {
-		libbpf_set_print(libbpf_perf_print);
-		libbpf_initialized = true;
-	}
+	err = libbpf_init();
+	if (err)
+		return ERR_PTR(err);
 
 	if (source) {
-		int err;
 		void *obj_buf;
 		size_t obj_buf_sz;
 
-- 
2.35.1

Re: [PATCH perf/core 3/5] perf tools: Move libbpf init in libbpf_init function

[Arnaldo Carvalho de Melo]

Em Fri, Apr 22, 2022 at 12:00:23PM +0200, Jiri Olsa escreveu:
> Moving the libbpf init code into single function,
> so we have single place doing that.

Cherry picked this one, waiting for Andrii to chime in wrt the libbpf
changes, if its acceptable, how to proceed, i.e. in what tree to carry
these?

- Arnaldo
 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/bpf-loader.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index b72cef1ae959..f8ad581ea247 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -99,16 +99,26 @@ static int bpf_perf_object__add(struct bpf_object *obj)
>  	return perf_obj ? 0 : -ENOMEM;
>  }
>  
> +static int libbpf_init(void)
> +{
> +	if (libbpf_initialized)
> +		return 0;
> +
> +	libbpf_set_print(libbpf_perf_print);
> +	libbpf_initialized = true;
> +	return 0;
> +}
> +
>  struct bpf_object *
>  bpf__prepare_load_buffer(void *obj_buf, size_t obj_buf_sz, const char *name)
>  {
>  	LIBBPF_OPTS(bpf_object_open_opts, opts, .object_name = name);
>  	struct bpf_object *obj;
> +	int err;
>  
> -	if (!libbpf_initialized) {
> -		libbpf_set_print(libbpf_perf_print);
> -		libbpf_initialized = true;
> -	}
> +	err = libbpf_init();
> +	if (err)
> +		return ERR_PTR(err);
>  
>  	obj = bpf_object__open_mem(obj_buf, obj_buf_sz, &opts);
>  	if (IS_ERR_OR_NULL(obj)) {
> @@ -135,14 +145,13 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
>  {
>  	LIBBPF_OPTS(bpf_object_open_opts, opts, .object_name = filename);
>  	struct bpf_object *obj;
> +	int err;
>  
> -	if (!libbpf_initialized) {
> -		libbpf_set_print(libbpf_perf_print);
> -		libbpf_initialized = true;
> -	}
> +	err = libbpf_init();
> +	if (err)
> +		return ERR_PTR(err);
>  
>  	if (source) {
> -		int err;
>  		void *obj_buf;
>  		size_t obj_buf_sz;
>  
> -- 
> 2.35.1

-- 

- Arnaldo

Re: [PATCH perf/core 3/5] perf tools: Move libbpf init in libbpf_init function

[Jiri Olsa]

On Fri, Apr 22, 2022 at 02:03:24PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Apr 22, 2022 at 12:00:23PM +0200, Jiri Olsa escreveu:
> > Moving the libbpf init code into single function,
> > so we have single place doing that.
> 
> Cherry picked this one, waiting for Andrii to chime in wrt the libbpf
> changes, if its acceptable, how to proceed, i.e. in what tree to carry
> these?

I think at this point it's ok with either yours perf/core or bpf-next/master,
I waited for them to be in sync for this ;-)

but as you pointed out there's issue with perf linked with dynamic libbpf,
because the current version does not have the libbpf_register_prog_handler
api that perf needs now.. also it needs the fix and api introduced in this
patchset

I'll check and perhaps we can temporirly disable perf/bpf prologue generation
for dynamic linking..? until the libbpf release has all the needed changes

jirka

> 
> - Arnaldo
>  
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/util/bpf-loader.c | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> > index b72cef1ae959..f8ad581ea247 100644
> > --- a/tools/perf/util/bpf-loader.c
> > +++ b/tools/perf/util/bpf-loader.c
> > @@ -99,16 +99,26 @@ static int bpf_perf_object__add(struct bpf_object *obj)
> >  	return perf_obj ? 0 : -ENOMEM;
> >  }
> >  
> > +static int libbpf_init(void)
> > +{
> > +	if (libbpf_initialized)
> > +		return 0;
> > +
> > +	libbpf_set_print(libbpf_perf_print);
> > +	libbpf_initialized = true;
> > +	return 0;
> > +}
> > +
> >  struct bpf_object *
> >  bpf__prepare_load_buffer(void *obj_buf, size_t obj_buf_sz, const char *name)
> >  {
> >  	LIBBPF_OPTS(bpf_object_open_opts, opts, .object_name = name);
> >  	struct bpf_object *obj;
> > +	int err;
> >  
> > -	if (!libbpf_initialized) {
> > -		libbpf_set_print(libbpf_perf_print);
> > -		libbpf_initialized = true;
> > -	}
> > +	err = libbpf_init();
> > +	if (err)
> > +		return ERR_PTR(err);
> >  
> >  	obj = bpf_object__open_mem(obj_buf, obj_buf_sz, &opts);
> >  	if (IS_ERR_OR_NULL(obj)) {
> > @@ -135,14 +145,13 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
> >  {
> >  	LIBBPF_OPTS(bpf_object_open_opts, opts, .object_name = filename);
> >  	struct bpf_object *obj;
> > +	int err;
> >  
> > -	if (!libbpf_initialized) {
> > -		libbpf_set_print(libbpf_perf_print);
> > -		libbpf_initialized = true;
> > -	}
> > +	err = libbpf_init();
> > +	if (err)
> > +		return ERR_PTR(err);
> >  
> >  	if (source) {
> > -		int err;
> >  		void *obj_buf;
> >  		size_t obj_buf_sz;
> >  
> > -- 
> > 2.35.1
> 
> -- 
> 
> - Arnaldo

Re: [PATCH perf/core 3/5] perf tools: Move libbpf init in libbpf_init function

[Andrii Nakryiko]

On Mon, Apr 25, 2022 at 12:25 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Fri, Apr 22, 2022 at 02:03:24PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Apr 22, 2022 at 12:00:23PM +0200, Jiri Olsa escreveu:
> > > Moving the libbpf init code into single function,
> > > so we have single place doing that.
> >
> > Cherry picked this one, waiting for Andrii to chime in wrt the libbpf
> > changes, if its acceptable, how to proceed, i.e. in what tree to carry
> > these?
>
> I think at this point it's ok with either yours perf/core or bpf-next/master,
> I waited for them to be in sync for this ;-)

I'd rather have all the libbpf code stay in bpf-next, otherwise Github
sync process becomes very hairy. BTW, I think libbpf v0.8 release is
pretty close, I plan to add few small features before cutting it, but
that should be done pretty soon

>
> but as you pointed out there's issue with perf linked with dynamic libbpf,
> because the current version does not have the libbpf_register_prog_handler
> api that perf needs now.. also it needs the fix and api introduced in this
> patchset
>
> I'll check and perhaps we can temporirly disable perf/bpf prologue generation
> for dynamic linking..? until the libbpf release has all the needed changes
>
> jirka
>
> >
> > - Arnaldo
> >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/perf/util/bpf-loader.c | 27 ++++++++++++++++++---------
> > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > >

[...]

[PATCH perf/core 4/5] perf tools: Register perfkprobe libbpf section handler

[Jiri Olsa]

Perf is using section name to declare special kprobe arguments,
which no longer works with current libbpf, that either requires
certain form of the section name or allows to register custom
handler.

Adding support for 'perfkprobe/' section name handler to take
care of perf kprobe programs.

The handler servers two purposes:
  - allows perf programs to have special arguments in section name
  - allows perf to use pre-load callback where we can attach init
    code (zeroing all argument registers) to each perf program

The second is essential part of new prologue generation code,
that's coming in following patch.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/bpf-loader.c | 50 ++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index f8ad581ea247..92dd8cc18edb 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -86,6 +86,7 @@ bpf_perf_object__next(struct bpf_perf_object *prev)
 	     (perf_obj) = (tmp), (tmp) = bpf_perf_object__next(tmp))
 
 static bool libbpf_initialized;
+static int libbpf_sec_handler;
 
 static int bpf_perf_object__add(struct bpf_object *obj)
 {
@@ -99,12 +100,61 @@ static int bpf_perf_object__add(struct bpf_object *obj)
 	return perf_obj ? 0 : -ENOMEM;
 }
 
+static struct bpf_insn prologue_init_insn[] = {
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_MOV64_IMM(BPF_REG_1, 0),
+	BPF_MOV64_IMM(BPF_REG_2, 0),
+	BPF_MOV64_IMM(BPF_REG_3, 0),
+	BPF_MOV64_IMM(BPF_REG_4, 0),
+	BPF_MOV64_IMM(BPF_REG_5, 0),
+};
+
+#define LIBBPF_SEC_PREFIX "perfkprobe/"
+
+static int libbpf_prog_prepare_load_fn(struct bpf_program *prog,
+				       struct bpf_prog_load_opts *opts __maybe_unused,
+				       long cookie __maybe_unused)
+{
+	size_t init_size_cnt = ARRAY_SIZE(prologue_init_insn);
+	size_t orig_insn_cnt, insn_cnt, init_size, orig_size;
+	const struct bpf_insn *orig_insn;
+	struct bpf_insn *insn;
+
+	/* prepend initialization code to program instructions */
+	orig_insn = bpf_program__insns(prog);
+	orig_insn_cnt = bpf_program__insn_cnt(prog);
+	init_size = init_size_cnt * sizeof(*insn);
+	orig_size = orig_insn_cnt * sizeof(*insn);
+
+	insn_cnt = orig_insn_cnt + init_size_cnt;
+	insn = malloc(insn_cnt * sizeof(*insn));
+	if (!insn)
+		return -ENOMEM;
+
+	memcpy(insn, prologue_init_insn, init_size);
+	memcpy((char *) insn + init_size, orig_insn, orig_size);
+	bpf_program__set_insns(prog, insn, insn_cnt);
+	return 0;
+}
+
 static int libbpf_init(void)
 {
+	LIBBPF_OPTS(libbpf_prog_handler_opts, handler_opts,
+		.prog_prepare_load_fn = libbpf_prog_prepare_load_fn,
+	);
+
 	if (libbpf_initialized)
 		return 0;
 
 	libbpf_set_print(libbpf_perf_print);
+	libbpf_sec_handler = libbpf_register_prog_handler(LIBBPF_SEC_PREFIX,
+							  BPF_PROG_TYPE_KPROBE,
+							  0, &handler_opts);
+	if (libbpf_sec_handler < 0) {
+		pr_debug("bpf: failed to register libbpf section handler: %d\n",
+			 libbpf_sec_handler);
+		return -BPF_LOADER_ERRNO__INTERNAL;
+	}
 	libbpf_initialized = true;
 	return 0;
 }
-- 
2.35.1

Re: [PATCH perf/core 4/5] perf tools: Register perfkprobe libbpf section handler

[Andrii Nakryiko]

On Fri, Apr 22, 2022 at 3:01 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Perf is using section name to declare special kprobe arguments,
> which no longer works with current libbpf, that either requires
> certain form of the section name or allows to register custom
> handler.
>
> Adding support for 'perfkprobe/' section name handler to take
> care of perf kprobe programs.
>
> The handler servers two purposes:
>   - allows perf programs to have special arguments in section name
>   - allows perf to use pre-load callback where we can attach init
>     code (zeroing all argument registers) to each perf program
>
> The second is essential part of new prologue generation code,
> that's coming in following patch.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/bpf-loader.c | 50 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index f8ad581ea247..92dd8cc18edb 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -86,6 +86,7 @@ bpf_perf_object__next(struct bpf_perf_object *prev)
>              (perf_obj) = (tmp), (tmp) = bpf_perf_object__next(tmp))
>
>  static bool libbpf_initialized;
> +static int libbpf_sec_handler;
>
>  static int bpf_perf_object__add(struct bpf_object *obj)
>  {
> @@ -99,12 +100,61 @@ static int bpf_perf_object__add(struct bpf_object *obj)
>         return perf_obj ? 0 : -ENOMEM;
>  }
>
> +static struct bpf_insn prologue_init_insn[] = {
> +       BPF_MOV64_IMM(BPF_REG_0, 0),
> +       BPF_MOV64_IMM(BPF_REG_1, 0),
> +       BPF_MOV64_IMM(BPF_REG_2, 0),
> +       BPF_MOV64_IMM(BPF_REG_3, 0),
> +       BPF_MOV64_IMM(BPF_REG_4, 0),
> +       BPF_MOV64_IMM(BPF_REG_5, 0),
> +};
> +
> +#define LIBBPF_SEC_PREFIX "perfkprobe/"

libbpf allows to register fallback handler that will handle any SEC()
definition besides the ones that libbpf handles. Would that work in
this case instead of adding a custom prefix handler here? See
prog_tests/custom_sec_handlers.c for example:

fallback_id = libbpf_register_prog_handler(NULL,
BPF_PROG_TYPE_SYSCALL, 0, &opts);

> +

[...]

Re: [PATCH perf/core 4/5] perf tools: Register perfkprobe libbpf section handler

[Jiri Olsa]

On Mon, Apr 25, 2022 at 11:22:54PM -0700, Andrii Nakryiko wrote:
> On Fri, Apr 22, 2022 at 3:01 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Perf is using section name to declare special kprobe arguments,
> > which no longer works with current libbpf, that either requires
> > certain form of the section name or allows to register custom
> > handler.
> >
> > Adding support for 'perfkprobe/' section name handler to take
> > care of perf kprobe programs.
> >
> > The handler servers two purposes:
> >   - allows perf programs to have special arguments in section name
> >   - allows perf to use pre-load callback where we can attach init
> >     code (zeroing all argument registers) to each perf program
> >
> > The second is essential part of new prologue generation code,
> > that's coming in following patch.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/util/bpf-loader.c | 50 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> > index f8ad581ea247..92dd8cc18edb 100644
> > --- a/tools/perf/util/bpf-loader.c
> > +++ b/tools/perf/util/bpf-loader.c
> > @@ -86,6 +86,7 @@ bpf_perf_object__next(struct bpf_perf_object *prev)
> >              (perf_obj) = (tmp), (tmp) = bpf_perf_object__next(tmp))
> >
> >  static bool libbpf_initialized;
> > +static int libbpf_sec_handler;
> >
> >  static int bpf_perf_object__add(struct bpf_object *obj)
> >  {
> > @@ -99,12 +100,61 @@ static int bpf_perf_object__add(struct bpf_object *obj)
> >         return perf_obj ? 0 : -ENOMEM;
> >  }
> >
> > +static struct bpf_insn prologue_init_insn[] = {
> > +       BPF_MOV64_IMM(BPF_REG_0, 0),
> > +       BPF_MOV64_IMM(BPF_REG_1, 0),
> > +       BPF_MOV64_IMM(BPF_REG_2, 0),
> > +       BPF_MOV64_IMM(BPF_REG_3, 0),
> > +       BPF_MOV64_IMM(BPF_REG_4, 0),
> > +       BPF_MOV64_IMM(BPF_REG_5, 0),
> > +};
> > +
> > +#define LIBBPF_SEC_PREFIX "perfkprobe/"
> 
> libbpf allows to register fallback handler that will handle any SEC()
> definition besides the ones that libbpf handles. Would that work in
> this case instead of adding a custom prefix handler here? See
> prog_tests/custom_sec_handlers.c for example:
> 
> fallback_id = libbpf_register_prog_handler(NULL,
> BPF_PROG_TYPE_SYSCALL, 0, &opts);

nice, I did not see that.. that should be better, no need for the prefix

thanks,
jirka

[PATCH perf/core 5/5] perf tools: Rework prologue generation code

[Jiri Olsa]

Some functions we use for bpf prologue generation are going to be
deprecated. This change reworks current code not to use them.

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

We use bpf_program__set_prep to hook perf callback before program
is loaded and provide new instructions with the prologue.

We workaround this by taking instructions for specific program,
attaching prologue to them and load such new ebpf programs with
prologue using separate bpf_prog_load calls (outside libbpf load
machinery).

Before we can take program instructions, we need libbpf to actually
load it. This way we get the final shape of its instructions (with
all relocations and verifier adjustments).

There's one glitch though.. perf kprobe program already assumes
generated prologue code with proper values in argument registers,
so loading such program directly will fail in the verifier.

That's where 'perfkprobe/' pre-load handler fits in and prepends
the initialization code to the program. Once such program is loaded
we take its instructions, cut off the initialization code and prepend
the prologue.

I know.. sorry ;-)

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/include/bpf/bpf.h                |   2 +-
 tools/perf/tests/bpf-script-example.c       |   2 +-
 tools/perf/tests/bpf-script-test-prologue.c |   2 +-
 tools/perf/util/bpf-loader.c                | 136 +++++++++++++++++---
 4 files changed, 120 insertions(+), 22 deletions(-)

diff --git a/tools/perf/include/bpf/bpf.h b/tools/perf/include/bpf/bpf.h
index b422aeef5339..91869f6fb672 100644
--- a/tools/perf/include/bpf/bpf.h
+++ b/tools/perf/include/bpf/bpf.h
@@ -50,7 +50,7 @@ static void (*bpf_tail_call)(void *ctx, void *map, int index) = (void *)BPF_FUNC
 #define SEC(NAME) __attribute__((section(NAME),  used))
 
 #define probe(function, vars) \
-	SEC(#function "=" #function " " #vars) function
+	SEC("perfkprobe/" #function "=" #function " " #vars) function
 
 #define syscall_enter(name) \
 	SEC("syscalls:sys_enter_" #name) syscall_enter_ ## name
diff --git a/tools/perf/tests/bpf-script-example.c b/tools/perf/tests/bpf-script-example.c
index ab4b98b3165d..56673fa1f30d 100644
--- a/tools/perf/tests/bpf-script-example.c
+++ b/tools/perf/tests/bpf-script-example.c
@@ -32,7 +32,7 @@ struct bpf_map_def SEC("maps") flip_table = {
 	.max_entries = 1,
 };
 
-SEC("func=do_epoll_wait")
+SEC("perfkprobe/func=do_epoll_wait")
 int bpf_func__SyS_epoll_pwait(void *ctx)
 {
 	int ind =0;
diff --git a/tools/perf/tests/bpf-script-test-prologue.c b/tools/perf/tests/bpf-script-test-prologue.c
index bd83d364cf30..00dac5a23938 100644
--- a/tools/perf/tests/bpf-script-test-prologue.c
+++ b/tools/perf/tests/bpf-script-test-prologue.c
@@ -26,7 +26,7 @@
 static void (*bpf_trace_printk)(const char *fmt, int fmt_size, ...) =
 	(void *) 6;
 
-SEC("func=null_lseek file->f_mode offset orig")
+SEC("perfkprobe/func=null_lseek file->f_mode offset orig")
 int bpf_func__null_lseek(void *ctx, int err, unsigned long _f_mode,
 			 unsigned long offset, unsigned long orig)
 {
diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index 92dd8cc18edb..10151da862c8 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -9,6 +9,7 @@
 #include <linux/bpf.h>
 #include <bpf/libbpf.h>
 #include <bpf/bpf.h>
+#include <linux/filter.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
@@ -49,6 +50,7 @@ struct bpf_prog_priv {
 	struct bpf_insn *insns_buf;
 	int nr_types;
 	int *type_mapping;
+	int *proglogue_fds;
 };
 
 struct bpf_perf_object {
@@ -56,6 +58,11 @@ struct bpf_perf_object {
 	struct bpf_object *obj;
 };
 
+struct bpf_preproc_result {
+	struct bpf_insn *new_insn_ptr;
+	int new_insn_cnt;
+};
+
 static LIST_HEAD(bpf_objects_list);
 static struct hashmap *bpf_program_hash;
 static struct hashmap *bpf_map_hash;
@@ -238,14 +245,31 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
 	return obj;
 }
 
+static void close_prologue_programs(struct bpf_prog_priv *priv)
+{
+	struct perf_probe_event *pev;
+	int i, fd;
+
+	if (!priv->need_prologue)
+		return;
+	pev = &priv->pev;
+	for (i = 0; i < pev->ntevs; i++) {
+		fd = priv->proglogue_fds[i];
+		if (fd != -1)
+			close(fd);
+	}
+}
+
 static void
 clear_prog_priv(const struct bpf_program *prog __maybe_unused,
 		void *_priv)
 {
 	struct bpf_prog_priv *priv = _priv;
 
+	close_prologue_programs(priv);
 	cleanup_perf_probe_events(&priv->pev, 1);
 	zfree(&priv->insns_buf);
+	zfree(&priv->proglogue_fds);
 	zfree(&priv->type_mapping);
 	zfree(&priv->sys_name);
 	zfree(&priv->evt_name);
@@ -480,9 +504,15 @@ static int
 parse_prog_config(const char *config_str, const char **p_main_str,
 		  bool *is_tp, struct perf_probe_event *pev)
 {
+	const char *main_str, *parse_str;
 	int err;
-	const char *main_str = parse_prog_config_kvpair(config_str, pev);
 
+	/* Make sure it's our section with 'perfkprobe/' prefix check. */
+	if (!strstarts(config_str, LIBBPF_SEC_PREFIX))
+		return -EINVAL;
+
+	parse_str = config_str + sizeof(LIBBPF_SEC_PREFIX) - 1;
+	main_str  = parse_prog_config_kvpair(parse_str, pev);
 	if (IS_ERR(main_str))
 		return PTR_ERR(main_str);
 
@@ -608,8 +638,8 @@ static int bpf__prepare_probe(void)
 
 static int
 preproc_gen_prologue(struct bpf_program *prog, int n,
-		     struct bpf_insn *orig_insns, int orig_insns_cnt,
-		     struct bpf_prog_prep_result *res)
+		     const struct bpf_insn *orig_insns, int orig_insns_cnt,
+		     struct bpf_preproc_result *res)
 {
 	struct bpf_prog_priv *priv = program_priv(prog);
 	struct probe_trace_event *tev;
@@ -657,7 +687,6 @@ preproc_gen_prologue(struct bpf_program *prog, int n,
 
 	res->new_insn_ptr = buf;
 	res->new_insn_cnt = prologue_cnt + orig_insns_cnt;
-	res->pfd = NULL;
 	return 0;
 
 errout:
@@ -765,7 +794,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
 	struct bpf_prog_priv *priv = program_priv(prog);
 	struct perf_probe_event *pev;
 	bool need_prologue = false;
-	int err, i;
+	int i;
 
 	if (IS_ERR_OR_NULL(priv)) {
 		pr_debug("Internal error when hook preprocessor\n");
@@ -803,6 +832,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
 		return -ENOMEM;
 	}
 
+	priv->proglogue_fds = malloc(sizeof(int) * pev->ntevs);
+	if (!priv->proglogue_fds) {
+		pr_debug("Not enough memory: alloc prologue fds failed\n");
+		return -ENOMEM;
+	}
+	memset(priv->proglogue_fds, -1, sizeof(int) * pev->ntevs);
+
 	priv->type_mapping = malloc(sizeof(int) * pev->ntevs);
 	if (!priv->type_mapping) {
 		pr_debug("Not enough memory: alloc type_mapping failed\n");
@@ -811,13 +847,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
 	memset(priv->type_mapping, -1,
 	       sizeof(int) * pev->ntevs);
 
-	err = map_prologue(pev, priv->type_mapping, &priv->nr_types);
-	if (err)
-		return err;
-
-	err = bpf_program__set_prep(prog, priv->nr_types,
-				    preproc_gen_prologue);
-	return err;
+	return map_prologue(pev, priv->type_mapping, &priv->nr_types);
 }
 
 int bpf__probe(struct bpf_object *obj)
@@ -924,6 +954,77 @@ int bpf__unprobe(struct bpf_object *obj)
 	return ret;
 }
 
+static int bpf_object__load_prologue(struct bpf_object *obj)
+{
+	int init_cnt = ARRAY_SIZE(prologue_init_insn);
+	const struct bpf_insn *orig_insns;
+	struct bpf_preproc_result res;
+	struct perf_probe_event *pev;
+	struct bpf_program *prog;
+	int orig_insns_cnt;
+
+	bpf_object__for_each_program(prog, obj) {
+		struct bpf_prog_priv *priv = program_priv(prog);
+		int err, i, fd;
+
+		if (IS_ERR_OR_NULL(priv)) {
+			pr_debug("bpf: failed to get private field\n");
+			return -BPF_LOADER_ERRNO__INTERNAL;
+		}
+
+		if (!priv->need_prologue)
+			continue;
+
+		/*
+		 * For each program that needs prologue we do following:
+		 *
+		 * - take its current instructions and use them
+		 *   to generate the new code with prologue
+		 * - load new instructions with bpf_prog_load
+		 *   and keep the fd in proglogue_fds
+		 * - new fd will be used in bpf__foreach_event
+		 *   to connect this program with perf evsel
+		 */
+		orig_insns = bpf_program__insns(prog);
+		orig_insns_cnt = bpf_program__insn_cnt(prog);
+
+		pev = &priv->pev;
+		for (i = 0; i < pev->ntevs; i++) {
+			/*
+			 * Skipping artificall prologue_init_insn instructions
+			 * (init_cnt), so the prologue can be generated instead
+			 * of them.
+			 */
+			err = preproc_gen_prologue(prog, i,
+						   orig_insns + init_cnt,
+						   orig_insns_cnt - init_cnt,
+						   &res);
+			if (err)
+				return err;
+
+			fd = bpf_prog_load(bpf_program__get_type(prog),
+					   bpf_program__name(prog), "GPL",
+					   res.new_insn_ptr,
+					   res.new_insn_cnt, NULL);
+			if (fd < 0) {
+				char bf[128];
+
+				libbpf_strerror(-errno, bf, sizeof(bf));
+				pr_debug("bpf: load objects with prologue failed: err=%d: (%s)\n",
+					 -errno, bf);
+				return -errno;
+			}
+			priv->proglogue_fds[i] = fd;
+		}
+		/*
+		 * We no longer need the original program,
+		 * we can unload it.
+		 */
+		bpf_program__unload(prog);
+	}
+	return 0;
+}
+
 int bpf__load(struct bpf_object *obj)
 {
 	int err;
@@ -935,7 +1036,7 @@ int bpf__load(struct bpf_object *obj)
 		pr_debug("bpf: load objects failed: err=%d: (%s)\n", err, bf);
 		return err;
 	}
-	return 0;
+	return bpf_object__load_prologue(obj);
 }
 
 int bpf__foreach_event(struct bpf_object *obj,
@@ -970,13 +1071,10 @@ int bpf__foreach_event(struct bpf_object *obj,
 		for (i = 0; i < pev->ntevs; i++) {
 			tev = &pev->tevs[i];
 
-			if (priv->need_prologue) {
-				int type = priv->type_mapping[i];
-
-				fd = bpf_program__nth_fd(prog, type);
-			} else {
+			if (priv->need_prologue)
+				fd = priv->proglogue_fds[i];
+			else
 				fd = bpf_program__fd(prog);
-			}
 
 			if (fd < 0) {
 				pr_debug("bpf: failed to get file descriptor\n");
-- 
2.35.1