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

[Nick Alcock]

On 17 Feb 2024, Kris Van Hees stated:
> On Fri, Feb 16, 2024 at 07:09:47PM +0000, Nick Alcock wrote:
>> I fell into exactly this trap myself, so I'll add some more comments
>> clarifying why we're doing this (and fixing the comment that wrongly
>> says that we are looking in /run/dtrace/probes/$pid/$prv/$mod/$fun/$prb,
>> which doesn't exist: it's $pid/$pid$prv.)
>
> There is no trap here.
>
> But there are multiple things going on here at the sane time:
>
> (1) The storage of DOF data which in its current design stores probe-specific
>     parsed DOF data in:
>
> 	/run/dtrace/stash/dof-pid/$pid/$dev-$ino/parsed/$prv$pid:$mod:$fun:$prb
>
>     And that is already known to be something that needs to change at the next
>     iteration to be a link to something like
>
> 	/run/dtrace/stash/dof/$dev-$ino/parsed/$prv:$mod:$fun:$prb
>
>     But even in its current design, using $prv$pid here makes no sense because
>     the DOF data is not an instantiated provider.  From a DOF perspective,
>     there is nothing pid (or task) specific about the parsed DOF.

I completely agree. Fixed! Sorry about that, I totally misunderstood.
(Actually I thought I was already using $prv:$mod:$fun$:prb in the
dof-pid dir. Whoops.)

>     Also, it would make sense to use a directory hierarchy for the probe data,
>     similar to what /run/dtrace/probes uses.  Then you could make the content
>     of /run/dtrace/probes/$pid/ simply be links to the $prv directory under
>     $dev-$ino/parsed and the rest of the probe specification hierarchy is
>     there already.  That is more efficient once you have DOF-per-mapping.

Yeah, I'll migrate to that at the same time as DOF-per-mapping hits.

> (3) The pid_probespec structure.  I'd still prefer to keep the pps_prv element
>     without the pid because this describes a probe at the meta-provider level,
>     i.e. independent from the task (pid).  It is primarily used to tell the
>     meta-provider (pid or usdt) to instantiate a probe (creating the
>     pid-specific provider if it does not exist yet).  It is the meta-provider
>     that actually decides how to name the task-specific provider.  So, at the
>     level of this structure, the provider name (pps_prv) should reflect the
>     name of the provider as given by the DOF (or "pid" for pid probes).

In my latest series (to be mailed out later this evening if the tests
all pass) I now have *two*, pps_prv and pps_prvpid.
dt_pid_create_one_probe sets pps_prv to "pid"; dt_pid_create_usdt_probes
sets pps_prvpid to the prv/pid value derived from the DOF stash: if
pps_prvpid is not set, provide_probe appends the PID, but if it is,
provide_probe uses it. The alternative is needlessly annoying: the
dt_pid_create_usdt_probes has to split the provider name and PID apart
only for provide_probe to stick it straight back on again!

Honestly I suspect pps_prv is unnecessary because it only ever has one
value, "pid", and we could move to a simple approach where pps_prvpid is
all we have (and if it's NULL we set the provider to "pid$pid"). Are we
likely to have a third fixed value that needs PID-appending?

> The only exception is the /run/dtrace/probes data because that specifically
> represents the probe names as DTrace sees them.

This was the only instance I remembered existed :)

>                                                   Ideally, that should be
> managed by DTrace processes instead of dtprobed but I do not believe that is
> in any way practical (due to concurrency issues, etc...)

Yeah, I couldn't see a way to make dtrace do that without much pain
either.

>                                                            This directory
> hierarchy exists purely for convenience anyway.  I'd still prefer we didn't
> use $prv$pid here because it really doesn't belong in dtprobed, but if it is
> limited to just this, might as well go with it for now.

It's only there because it makes probe wildcard matching so much easier.
The whole /run/dtrace/probes dir is not strictly stash-related, but,
well, "making life easier for dtrace in re usdt probes" is what dtprobed
is really for and this fits in that remit.

-- 
NULL && (void)