[DTrace-devel] [PATCH 02/10] dtprobed: add the DOF stash

[Kris Van Hees]

On Wed, Aug 30, 2023 at 02:22:03PM +0100, Nick Alcock wrote:
> On 29 Aug 2023, Nick Alcock via DTrace-devel said:
> 
> > On 29 Aug 2023, Kris Van Hees stated:
> >> handle struct changes.  Alternatively, you could just leave the parsing to
> >> DTrace itself and only store the raw DOF along with the helper info, right?
> >
> > Hmm. As long as DTrace jails the parser too (which is fine, it can just
> > use the exact same code as the daemon does now), that dooes seem like a
> > *much* better approach. I don't think the daemon needs the result of
> > parsing at all any more: we literally just suck it in and stash it, so
> > DTrace could just as easily do it all itself. I'm kicking myself for not
> > noticing that its need for the parsed output had fallen to zero. (The
> > stash still needs maintenance, but only of the raw stuff and it should
> > be a bit simpler.)
> >
> > Not a small change though. We can converge on that in the next release,
> > perhaps? I doubt it would take less than a week to do even if I had
> > nothing else on my plate.
> 
> I am still musing on this, trying to figure out improvements which
> retain the ability to parse DOF only once per mapping that doing the
> parsing in dtprobed has. Here's my current idea (largely your idea, with
> elaborations):

Not at all my idea though.  I am *not* in any way proposing parsing the DOF in
dtrace or using a child process forked by dtrace.  I absolutely prefer to have
the parsing take place in dtprobed.

>  - dtprobed just listens for DOF-contributing ioctls and maintains the
>    DOF stash, with raw DOF and a per-pid per-mapping dh and everything
>    necessary to detect outdated DOF (the exec-mapping file, etc). No
>    parsing of the DOF at all. (Trivial change, actually already did it
>    in my tree.)
> 
>  - when dtrace needs to get DOF for some process to find likely probes,
>    it tries to open the parsed representation of the DOF out of the
>    stash first. It won't find it because nothing has written it out: so
>    it forks off a jailed parser child and does the parsing itself,
>    writing out the parsed DOF to the same place in the stash that
>    dtprobed does now. We can use the same struct-versioning scheme as
>    now: it's just that rather than doing some sort of aggressive
>    reparsing, dtrace just deletes the parsed representation in the DOF
>    stash if it finds it's incompatible, and reparses it as if it was
>    never there. Difficulty: a couple of hours, max, and a net code
>    saving. All the code is already there and was written in the
>    expectation that dtrace might end up using it instead of dtprobed.

The complexity of how to handle DOF being added and being removed independent
from dtrace's parsing is not addressed and can be a problem.  What if in a
rare situation dtrace is upgraded while some dtraces are still running - will
a new one (seeing incompatible data) wipe out the parsed representation of the
old one?  What if the old one was in the middle of writing it?  WHat is the
impact of forking, reading, parsing, writing, and then reading the resulting
data for the future case of systemwide probing where this would have to happen
while dtrace is actively consuming probe data?  How is the situation handled
of two dtrace instances trying to parse the data at the same time?  Could one
end up reading partial or corrupt data?  Etc..

I honestly do not understand why we would go away from the original design of
doing the parsing in dtprobed *which it is already doing*.  What is gained by
having dtrace itself do it?  I initially did not like that we needed a daemon,
but I was convincedt is necessary.  It took quite some back and forth talking
but a decent design came out of that.  I honestly do not see any good argument
to move away from that.  I.e. I see no real benefit to moving the parsing to
dtrace.

> That's it! It's easy enough that I can probably do it in the next day or
> two. Future developments below.
> 
>  - the above gives you the efficiency benefits of parsing only once
>    while entirely avoiding parsing DOF for processes that never get
>    traced, so actually a big efficiency gain, with no need for a textual
>    serialization format or anything. The only possible efficiency loss
>    happens when we do systemwide tracing and suddenlyj dtrace needs to
>    know about all probes in all running processes. In that case, we
>    rejig the jailed parser to be a pool of parsers, and poll() on it so
>    we can rapidly traverse all unparsed DOF in (affected processes in)
>    the stash and parse the lot, parsing multiple DOFs in parallel as
>    needed. Difficulty: higher, but is part of systemwide probing which
>    is a bigger change in itself.

You already pointed out that the parsing is very fast to the point where you
don't consider it an issue.  So, why not keep the parsing in dtprobed, write
out data *per probe* so it is easy for dtrace to see what probes exist (i.e.
support convenient probe matching based on provider:module:function:probe) with
minimal data accecss, and dtrace handles the probe creation/destruction.  That
is what I suggested before and I still do not see where your suggestion here is
significantly better.  I think the tiny impact of parsing all DOF that comes in
(even if no dtrace ever uses it) is quite preferable over adding forked
seccomp jailed children in dtrace processes.

>  - we could also revive the code that lets us extract DOF from processes
>    even before their constructors run, but figuring out the dh there
>    seems likely to be a bit harder and the benefits (probes in ld.so and
>    in very early ELF constructors) are marginal enough that I think we
>    should put this off until neeeded :)

Yes, that is not something we need to worry about at this point.