[DTrace-devel] restoring how DTrace adds probes

[Kris Van Hees]

On Mon, Jun 08, 2020 at 05:56:18PM -0700, Eugene Loh wrote:
> I'll try drafting something from scratch that is shorter (no background 
> discussion) and takes your feedback into account. First, just a few 
> questions.
> 
> On 06/08/2020 02:21 PM, Kris Van Hees wrote:
> > You are missing an important step...  legacy DTrace would open the
> > /dev/dtrace/dtrace pseudo-device file to obtain a file descriptor to issue
> > ioctl requests on.  That open request got handled by the dtrace kernel module,
> > and one of the first things it does is:
> > 	dtrace_probe_provide(NULL, NULL);
> 
> How would you feel about replacing the current dt_provimpl_t member
>          int (*populate)(dtrace_hdl_t *dtp);
> with
>          int (*populate)(dtrace_hdl_t *dtp, const dtrace_probedesc_t *spec)
> One could call it with spec==NULL to get "all" (pre-defined) probes or 
> with non-NULL spec to try to create a probe on demand.
> 
> And, what about renaming from "populate" to "provide" to connect more 
> directly to the legacy implementation?

I lean more towards having two distinct functions because they do quite
different things.  One (the existing one) called populate() and one called
provide() perhaps.

The legacy implementation performs the creation of probes based on the probe
description as part of the probe match operation because it has to.  Since the
probes list is maintained at the kernel level it is shared by all instances of
dtrace.  If one causes additional probes to be created, the other dtrace
instances will be able to resolve that probe as well.  Hence the choice to
perform a probe match prior to loading programs.  We no longer have that
situation (probe list is maintained per instance)

> >> *)  We should clean up these two functions:
> >>           - dtrace_probe_iter()
> >>           -     dt_probe_iter()
> >> Once again, we should revert somewhat to the legacy implementation.
> >> That is, dt_probe_iter() used to be a static function that was used
> >> simply as a callback within dtrace_probe_iter().  In the current
> >> version, dt_probe_iter() is no longer a static function.  On the other
> >> hand, it is still used only by dtrace_probe_iter().  And
> >> dtrace_probe_iter() has become simply a wrapper for dt_probe_iter().
> >> Therefore:
> >>       - move probe iteration from dt_probe_iter() into dtrace_probe_iter()
> >>       - if dt_probe_iter() is still needed, make it static once again
> > That code is still in need of further refining as we move towards supporting
> > wildcard probe descriptions in full.  So, we will need to evaluate the
> > interacton between dtrace_probe_iter and dt_probe_iter().  Note that the
> > dtrace_probe_iter() is the function that is exported as part of the libdtrace
> > API whereas dt_probe_iter() provides the implementation that is planned to be
> > called from other parts of libdtrace itself,
> 
> Okay, I suppose, but just to be clear:  is this new?  In both legacy and 
> v2, dt_probe_iter() is used only by dtrace_probe_iter(), while 
> dtrace_probe_iter() is called both from outside (cmd/dtrace.c) and 
> inside (dt_probe_info) libdtrace.

Well, in the legacy version, there is a distinct difference between the two
functions, and in fact, one cannot just be inserted into the other because
dt_probe_iter() is passed as a callback function to dt_idhash_iter().

In DTrace v2 we do things a bit different and we no longer use the idhash code
to perform matching lookups for probes.  Right now it would be tempting to
integrate dt_probe_iter() into dtrace_probe_iter() but in the upcoming work to
support wildcard probe descriptions in clauses we will need to perform callback
calls for matching probes based on the probe struct itself rather than only
the description.  That is why the current implementation was introduced in
DTrace v2.  It retains API compatibility for DTrace (since dtrace_probe_iter()
is an exported PAU functions) while being able to change the underlying
implementation.