[DTrace-devel] [PATCH dtrace 0/3] fprobe fallback kprobe support, sched fix

[Kris Van Hees]

On Thu, Oct 10, 2024 at 10:03:37PM +0100, Alan Maguire wrote:
> hi Kris
> 
> thanks for the detailed explanation! Replies below..
> 
> On 09/10/2024 18:34, Kris Van Hees wrote:
> > Hi Alan,
> > 
> > I see what you are doing here and I think that it certainly has merit, but I
> > would say that the implementation is breaking some assumptions that are part
> > of the provider design in the code base.  The way that a probe is related to
> > a provider implementation is rather fundamental and you end up breaking that
> > by having a fprobe-backed probe actually use the kprobe implementation.  I am
> > thinking how we can establish the same without breaking that association.
> > 
> > There is also the fact to consider that if we end up attaching a given probe
> > both as an fprobe-backed FBT probe and a kprobe-based FBT probe, things can
> > get really interesting because the defined order of execution of clauses will
> > no longer be satisfied.
> > 
> > Now, what perhaps could work is if we were to first try to determine whether
> > we can use a fprobe-backed for an FBT probe, and if that is not possible, do
> > a fall-back to a kprobe-backed probe by removing the probe and re-inserting it
> > as a kprobe-backed FBT probe.  Since a probe is associated with a provider by
> > a pointer to the provider implementation, that should work.
> > 
> > It should also still work for probe lookup (though that should no longer be
> > taking place at that point in processing) because the lookup code is based
> > on the provider name selecting the correct bucket, which can contain probes
> > that have that same provider name even if the implementation for them is
> > different.
> > 
> > This way we can never have a probe that is associated with the fprobe
> > implementation but really ends up using the kprobe implementation.  The
> > association between probe and provider implementation would still be kept
> > as-is.
> > 
> > Do you think that would work?
> 
> I've been looking at the code, and the problem I see is that it's hard
> to find a place where we can look at the actually _used_ probes (as
> opposed to those we populate()) and make determinations around kprobe
> support. My first go at this was simply to use the kprobe implementation
> for all functions containing a ".". However, as you observed, that could
> potentially result in a mix of fprobe and kprobe for a particular probe
> context which isn't allowed.
> 
> And for programs where a probe participates in multiple contexts we'd
> need to worry about whether probe replacement in one then had knock-on
> effects for fprobe/kprobe mixing, timing etc. Specifically if we
> replaced a fprobe by a kprobe for one clause, we'd need to do the same
> for it in other clauses I think.

Yes, but note that the compilation of clauses does not actually involve
knowing anything about the probe implementation.  So that is no issue.
And since clauses for a particular probe are associated with the very
probe, replacing it (or perhaps moving it to another implementation) is
automatically going to affect all clauses associated with it.

> So I _think_ we're stuck with an either-or; either we can use fprobes
> everywhere, or we have to use kprobes everywhere in a program. Is that
> the case do you think?

Yes, but mostly for other reasons...  DTrace guarantees the execution of
clauses in program-order.  And that can only be provided for in BPF if we
execute the clauses for a particular probe within a single BPF program.
So, mixing kprobe and fprobe based instances of the same FBT probe would
result in 2 BPF programs, and thereby violate the clause order guarantee.

> If so, the question then becomes is there a way to iterate over the
> actually used fbt probes to make sure they are usable as fprobes; if not
> replacing all probes with a kprobe backend. The problem is the only
> place I see that is in the compile of the various clauses. We'd need (I
> think) some kind of prepare phase where we walk the clauses and figure
> out which probes are associated to make this determination. Once we
> start compiling clauses, I think it's too late to backpedal and replace
> an implementation.

Actually, on systems where fprobes are available, the probe definition (name,
arguments) won't change so changing the underlhing implementation won't be
an issue at the time of compiling the clauses.  It is only when we generate
the trampolines that it matters which implementation is being used.  So, we
can change the implementation after we know what the clauses need.

> Or are there other places/ways to solve this I'm missing? Thanks!
> 
> Alan
> 
> > ONe complication I can think of is cases where both func and func.<suffix>
> > should be traced when you use an fbt::func:entry (or return) probe.  They
> > would both need to be done as kprobes, because (again) we cannot have a mix.
> > 
> > On Wed, Oct 09, 2024 at 03:02:33PM +0100, Alan Maguire wrote:
> >> This series is focused on solving a few issues with fprobe-based
> >> attachment which prevent us being able to attach to functions
> >> like finish_task_switch.isra.0.  Such functions are present in
> >> available_filter_functions, but because they either lack BTF
> >> representations or those representations are named without the
> >> .isra suffix, attachment via fentry/fexit is currently impossible.
> >> Falling back to kprobe attach is the best solution here.
> >>
> >> However providers have been written with the implicit assumption
> >> that certain aspects of the probes are shared across the provider;
> >> program type, stack skips etc.  Patch 1 addresses this by providing
> >> optional provider implementation callbacks to return these values
> >> for a specific probe.  Patch 2 then updates the fbt provider to
> >> use these mechanisms to support fallback to kprobe and to support
> >> "."-suffixed functions.  Finally patch 3 can then specify
> >> fbt::finish_task_switch*:return as the underlying probe for
> >> sched:::on-cpu.
> >>
> >> Tested on upstream, UEK7 (5.15-based kernel) and UEK6 (5.4-based).
> >>
> >> Alan Maguire (3):
> >>   dtrace: add support for probe-specific prog types, stack skips
> >>   fbt: support ".isra.0" suffixed functions
> >>   sched: fix on-cpu firing for kernels < 5.16
> >>
> >>  libdtrace/dt_bpf.c         |  4 +-
> >>  libdtrace/dt_cc.c          |  2 +-
> >>  libdtrace/dt_probe.c       | 16 ++++++++
> >>  libdtrace/dt_probe.h       |  2 +
> >>  libdtrace/dt_prov_fbt.c    | 84 +++++++++++++++++++++++++++++++-------
> >>  libdtrace/dt_prov_rawtp.c  |  2 +-
> >>  libdtrace/dt_prov_sched.c  | 23 +----------
> >>  libdtrace/dt_prov_sdt.c    |  2 +-
> >>  libdtrace/dt_provider.h    |  4 ++
> >>  libdtrace/dt_provider_tp.c | 12 +++++-
> >>  libdtrace/dt_provider_tp.h |  9 +++-
> >>  11 files changed, 117 insertions(+), 43 deletions(-)
> >>
> >> -- 
> >> 2.43.5
> >>