On Wed, Feb 28, 2024 at 4:43 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, Feb 23, 2024 at 1:32 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
> >
> > On Tue, Feb 13, 2024 at 01:09:54PM +0100, Jiri Olsa wrote:
> > > On Mon, Feb 12, 2024 at 08:06:06PM -0800, Andrii Nakryiko wrote:
> > >
> > > SNIP
> > >
> > > > > > But the way you implement it with extra flag and extra fd parameter
> > > > > > makes it harder to have a nice high-level support in libbpf (and
> > > > > > presumably other BPF loader libraries) for this.
> > > > > >
> > > > > > When I was thinking about doing something like this, I was considering
> > > > > > adding a new program type, actually. That way it's possible to define
> > > > > > this "let's skip return probe" protocol without backwards
> > > > > > compatibility concerns. It's easier to use it declaratively in libbpf.
> > > > >
> > > > > ok, that seems cleaner.. but we need to use current kprobe programs,
> > > > > so not sure at the moment how would that fit in.. did you mean new
> > > > > link type?
> > > >
> > > > It's kind of a less important detail, actually. New program type would
> > > > allow us to have an entirely different context type, but I think we
> > > > can make do with the existing kprobe program type. We can have a
> > > > separate attach_type and link type, just like multi-kprobe and
> > > > multi-uprobe are still kprobe programs.
> > >
> > > ok, having new attach type on top of kprobe_multi link makes sense
> >
> > hi,
> > I have further question in here.. ;-)
> >
> > so I implemented the new behaviour on top of new attach_type, but I keep
> > thinking that it's the 'same/similar logic' as if I added extra flag to
> > attr.link_create.kprobe_multi.flags, which results in much simpler change
> >
> > is following logic breaking backward compatibility in any way?
>
> Even if it doesn't break compatibility, isn't new attach_type better
> to explicitly show that semantics is completely different (it's two
> BPF program executions, before and after, for each specified kprobe),
> isn't that different enough? We can also have a different set of
> helpers for this kwrapper/uwrapper program type (probably could do it
> through flags as well, but attach_type again seems a better fit
> here..)?
>
> What makes it harder if you are doing it as a new attach_type?
I just looked at the RFC patches you sent, and I guess it looks fine
to me with the flag as well, given kretprobe.multi is also specified
through flags.
>
> >
> > - having new flag in attr.link_create.kprobe_multi.flags
> > - that force the attach of the bpf program to entry/exit function probes
> > - and the kprobe entry program return value now controls invocation
> > on the exit probe
> >
> > so the new flag changes the semantics of the entry program return value,
> > which seems fine to me, because it depends on the new flag.. do I miss
> > any other problem with that?
> >
> > thanks,
> > jirka
> >
> > >
> > > >
> > > > >
> > > > > > You just declare SEC("kprobe.wrap/...") (or whatever the name,
> > > > > > something to designate that it's both entry and exit probe) as one
> > > > > > program and in the code there would be some way to determine whether
> > > > > > we are in entry mode or exit mode (helper or field in the custom
> > > > > > context type, the latter being faster and more usable, but it's
> > > > > > probably not critical).
> > > > >
> > > > > hum, so the single program would be for both entry and exit probe,
> > > > > I'll need to check how bad it'd be for us, but it'd probably mean
> > > > > just one extra tail call, so it's likely ok
> > > >
> > > > I guess, I don't know what you are doing there :) I'd recommend
> > > > looking at utilizing BPF global subprogs instead of tail calls, if
> > > > your kernel allows for that, as that's a saner way to scale BPF
> > > > verification.
> > >
>
> [...]
