[DTrace-devel] [PATCH 10/15] Change condition to bypass trampoline generation

[Kris Van Hees]

On Thu, Feb 23, 2023 at 01:03:52PM +0000, Nick Alcock wrote:
> On 23 Feb 2023, Kris Van Hees via DTrace-devel verbalised:
> 
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> 
> What's the reason for this change? The only reasons I can see for it
> make no sense: that you want to be able to generate trampolines for code
> for which trampoline == NULL (which is a crash condition) or that you
> want to skip trampoline generation for code that has trampolines (?! why?)
> 
> Needs more justification :)
> 
> > ---
> >  libdtrace/dt_bpf.c         | 2 +-
> >  libdtrace/dt_prov_uprobe.c | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > index a4d5d664..23255a8d 100644
> > --- a/libdtrace/dt_bpf.c
> > +++ b/libdtrace/dt_bpf.c
> > @@ -1092,7 +1092,7 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
> >  		 * Enabled probes with no trampoline act like they exist but
> >  		 * no code is generated for them.
> >  		 */
> > -		if (prp->prov->impl->trampoline == NULL)
> > +		if (prp->prov->impl->prog_type == BPF_PROG_TYPE_UNSPEC)
> >  			continue;
> 
> This was there for a reason: if something doesn't have a trampoline we
> don't want to do anything that relies on it. You're replacing this with
> the implicit assumption that only UNSPEC-type providers will ever not
> have a trampoline, and that's not guaranteed, it's just something that
> happens to be true right now.

It is actually the other way around.  We started with the notion that providers
always need a trampoline and that was true because all programs were attached
as BPF programs to real probes.

Then we added a group of probes that use underlying probes to do the real work,
but the overlying and under;ying probes had the same DTrace context and so we
ended up with providers that do not need a trampoline.  And so that ended up
introducing the change to only call the trampoline function when there actually
is one.  But in retrospect that was a bit of a shortsighted idea, because...

... we now have another case where regular trampoline generation should not be
taking place because we are not generating a program that will be attached to
a real probe but we do need to generate a trampoline to set up the specific
DTrace context for the dependent probe.

The common thing here is that both groups of providers are not actually
representing real probes, which ccan be specified by setting the prog_type to
be BPF_PROG_TYPE_UNSPEC.  Therefore, this patch uses that as an explicit way
to indicate that program load should not generate a trampoline for this probe.

We need to be able to support providers that *have* a trampoline function *and*
do not want it called during program load.  This does that.

I'll update the commit message explaining this.

r-b OK with that explanation?