On Thu, Jan 13, 2022 at 7:14 AM Jiri Olsa <jolsa@xxxxxxxxxx> 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
>
