[DTrace-devel] [PATCH v3 08/21] dtprobed, usdt: move usdt probe creation/deletion to dtrace

[Nick Alcock]

On 15 Feb 2024, Kris Van Hees said:

> More comments... (previous ones were primarily due to needing to look at this
> code and the dof-stash patch together to figure thigns out)

Ack!

> It will get mentioned again below, but this patch really is multiple patches
> in one.  I would rather have seen it as separate patches so it would be more
> clear.  E.g. the code to write data to the DOF stash when it is processed can
> be its own patch, while still keeping the code that creates the uprobes.

... so a split where we start out by writing out the DOF stash but still
doing everything else as before, then migrate to using the DOF stash as
a second half?

> And then the patch that makes dtrace do the probe creation based on the DOF
> stash would be the place to remove the uprobe creation from dtprobed and do
> it in dtrace itself based on the DOF stash.  (And other things that are
> separate patches are listed below.)

... seems reasonable. Splitting accordingly (it's turning out to be a
bit of a nightmare, but it needs doing so I'm doing it anyway, learning
a lot about obscure git diff flags in the meantime, and doing a bunch of
manual patch reconstruction).

> On Wed, Feb 14, 2024 at 04:02:42PM -0500, Kris Van Hees via DTrace-devel wrote:
>> On Tue, Jan 16, 2024 at 09:13:04PM +0000, Nick Alcock via DTrace-devel wrote:
>> >  - it doesn't scale: the new probe registration/removal grinds to a halt
>> >    after a few tens of thousands of uprobes are registered
>
> I would add here that another scalability issue lies in the fact that with
> dtprobed doing it, you would end up with uprobe sbeing created for every USDT
> probe that *could* be traced. so often lots of USDT probes might get created
> that were not being used at all.

Ack! Noted.

>> > The solution to this is to remove all the uprobe registration code from
>> > dtprobed (goodbye, libcommon/uprobes.c) and migrate it all into
>> > libdtrace/dt_prov_uprobe.c.  This is convenient because it's where PID probe
>> > registration already happens, and it turns out we can reuse almost all the
>> > code (and it even fixes a pre-existing bug, with uprobes naturally being
>> > named based on inode-relative offset rather than address).
>
> Um, that was fixed in commit 8e1bdb8d16 ("uprobes: name uprobes based on offset
> in inode rather than address") which I introduced Oct 24, so that bug was
> already fixed well before this patch.  I don't think there is a point in
> mentioning something that was already fixed well before all this.

Yeah, I think we fixed it more or less simultaneously and then I removed
the code from here when you merged yours to dev and forgot to update the
commit log. (Fixed.)

>> > As for DTrace itself, we keep the underlying/overlying probe distinction
>> > from the dtprobed-uprobe-creation days, because one distinction which *does*
>> > still exist is that pid probes are created by fiat of the dtrace user, while
>> > usdt probes are restricted to those baked into the program: the "something
>> > in dt_pid.c creates underlying probes corresponding to USDT probes" stuff
>> > still works. But the method we use to find the USDT probes changes. Instead
>> > of pre-scanning the /sys/kernel/debug/tracing/uprobe_events list and then
>> > looking for probes that relate to the current process, we can just scan the
>> > dof-pid directory in the DOF stash that relates to the specific PID we are
>> > interested in, pull in the DOF, and generate probes from it.  Much simpler
>> > and much less work.
>
> I think this could use a bit or work to rephrase.  The underlying/overlying
> thing existed before USDT came on the scene (albeit in a slightly different
> form).  But no need to mention any of that.  This patch itself does not really
> make a difference in this area, so I think this paragraph should probably
> limit itself to just describing how things change in terms of how we know what
> USDT probes are availabnle for probing.  I.e. instead of dtprobed creating
> uprobes for all USDT probes for running processes thar could get probed, and
> dtrace scanning through those, dtrace can now scan the DOF stash and determine
> what uprobes it needs to create for USDT probes it is interested in.

Ack, fixed (you'll see the fix when I manage to finish the splits and
push it). Also it's not using the dof-pid directory any more :)

>> > But the pre-parsing has one tiny caveat. All the DOF we need is already
>> > pre-parsed via the seccomp-jailed child of dtprobed and written to
>> > files under /run/dtrace/stash/probes/$pid, but if dtprobed was upgraded
>> > since this instance of DTrace started, and the new dtprobed has a newer
>> > struct dof_parsed, we want to *ignore* any such (too-new) parsed DOF and not
>> > register the corresponding probes.  The previous commit introduced a new
>> > DOF_PARSED_VERSION #define which is bumped whenever struct dof_parsed
>> > changes and gets stuck at the start of all parsed DOF in the DOF stash: we
>> > just need to compare that to what's baked into this copy of DTrace, and
>> > avoid using any parsed DOF for which those are different.
>
> This is overly verbose ;) but ok.

Me, overly verbose? Never! I was just puffing out my chest about the
seccomp-jailing. Dropped that bit, who cares how it's parsed, what
matters is that it's already been parsed by something.

>> > Right now this is nearly impossible to observe, because the DOF is almost
>> > entirely scanned at startup, except for incoming dlopen()s (we don't observe
>> > newly-started processes or anything): but a long-running dtrace watching a
>> > process that is experiencing dlopen()s might still see this. In future, when
>> > systemwide tracing is implemented and we can observe new processes with DOF
>> > appearing while DTrace runs, this will become more significant.
>
> I would drop this paragraph.  You already describe the mechanism in the
> previous paragraph which is sufficient.  Talking about systemwide tracing in
> this commit is not really useful I think, and also speculates on future
> enhancement immplementation details (and even whether that feature will make
> it into DTrace anytime soon or ever).  Just leave it out.

True! I was more musing aloud about ways this might actually happen
given that nearly all probes are parsed at dtrace startup anyway. It's
quite unlikely :)

>> > Other tweaks in this commit:
>> > 
>> >  - rationalize the parser_in_pipe/parser_out_pipe stuff in dtprobed: rather
>> >    than being arrays, this is all hidden and the parser_in_pipe is simply
>> >    the thing we read from while the parser_out_pipe is what we write to.
>
> Doesn't need to be mentioned here I think.

Yeah, too minor. Added a (beyond minor cleanups) to cover those.

>> >  - move all the (dev, ino) determination out of process_dof() so we can
>> >    use it as the reparsing function when reparse_dof() is called, as well
>> >    as using it to do the initial parsing; at reparse time the (dev, ino)
>> >    comes from directory names in the DOF stash, not inspection of the actual
>> >    mappings via the libproc API
>
> Hm, this makes me think tht this reparsing support really shouldn't be in
> this patch.  That seems to be something tht would have been better in its
> own patch.

This is the real nightmare bit, but honestly this is git's fault and I
need to learn how to fix up cases when git diff produces useless
horribel interspersed output like this *anyway*. Fixing it currently
requires hand partial-hunk-application :/ oh well, it's only a dozen or
so lines.

>> >  - the FUSE loops were buggy and didn't handle dtprobed getting hit by a
>> >    signal well: we were spotting -EINTR when a fuse_session_receive_buf()
>> >    happened, but dtprobed spends almost all its time blocked on poll() and
>> >    that wasn't handling -EINTR at all.  Now we're planning to intentionally
>> >    hit dtprobed with signals during testing, this needs fixing.
>
> This would have fit nicely in its own patch - and important enough to be its
> own patcj, actually.

Split.

>> >  - add a -s option to dtprobed to allow the DOF stash directory to be
>> >    set. This is mostly going to be used by in-tree testing, which points the
>> >    DOF stash to a directory under build/ to avoid collisions with any
>> >    dtprobed that may be running systemwide.  DTrace gains a new dofstashpath
>> >    option for the same reason.  There is no need to document these: like
>> >    dtprobed's -n option and the DTRACE_DOF_INIT_DEVNAME environment
>> >    variable, these are really only testsuite internal implementation
>> >    details.
>
> This should also be its own patch.

Split!