Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 18 Feb 2022 14:03:25 +0100
From:   Jiri Olsa <olsajiri@gmail.com>
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     Jiri Olsa <jolsa@kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Andrii Nakryiko <andrii@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Ingo Molnar <mingo@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Ian Rogers <irogers@google.com>,
        "linux-perf-use." <linux-perf-users@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH 3/3] perf tools: Rework prologue generation code
Message-ID: <Yg+ZHUm4raVBwnQP@krava>
References: <20220217131916.50615-1-jolsa@kernel.org>
 <20220217131916.50615-4-jolsa@kernel.org>
 <CAEf4BzYP7=JuyuY=xZe71urpxat4ba-JnqeSTcHF=CYmsQbofQ@mail.gmail.com>
 <Yg9geQ0LJjhnrc7j@krava>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yg9geQ0LJjhnrc7j@krava>
Precedence: bulk

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

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

jirka