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?
>
> - 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.
> >
[...]
