[DTrace-devel] [PATCH 04/13] Add ability to provide user-specified probes on demand

[Kris Van Hees]

On Mon, Jul 06, 2020 at 09:32:45AM -0700, Eugene Loh wrote:
> On 07/06/2020 08:31 AM, Kris Van Hees wrote:
> 
> > On Wed, Jul 01, 2020 at 10:41:09PM -0400, eugene.loh at oracle.com wrote:
> >> From: Eugene Loh <eugene.loh at oracle.com>
> >>
> >> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> >> ---
> >>   libdtrace/dt_probe.c    | 20 ++++++++++++++++++++
> >>   libdtrace/dt_provider.h |  2 ++
> >>   2 files changed, 22 insertions(+)
> >>
> >> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> >> index 46c47fbe..be407dd2 100644
> >> --- a/libdtrace/dt_probe.c
> >> +++ b/libdtrace/dt_probe.c
> >> @@ -1141,6 +1141,26 @@ dt_probe_iter(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp,
> >>   
> >>   	tmpl.desc = pdp;
> >>   
> >> +	/*
> >> +	 * Special case: if the probe name is specified,
> >> +	 * give all providers the opportunity to provide it.
> >> +	 */
> >> +	if (n_is_glob == 0) {
> >> +		dt_provider_t *pvp;
> >> +		dtrace_probedesc_t pd;
> >> +
> >> +		/* loop over providers */
> >> +		for (pvp = dt_list_next(&dtp->dt_provlist);
> >> +		    pvp != NULL; pvp = dt_list_next(pvp)) {
> >> +			if (pvp->impl->provide == NULL ||
> >> +			    !dt_gmatch(pvp->desc.dtvd_name, pdp->prv))
> >> +				continue;
> >> +			memcpy(&pd, pdp, sizeof (pd));
> >> +			pd.prv = pvp->desc.dtvd_name;
> >> +			pvp->impl->provide(dtp, &pd);
> >> +		}
> >> +	}
> >> +
> > You introduce a limitation here that did not exist in DTrace v1 and I do not
> > see a reason why we would be more restrictive than DTrace v1.  Why would we not
> > allow globs in the probe name?  Any filtering on probe descriptions that a
> > provider might be able to work with (nor not) should be at the level of the
> > provider.
> 
> Good point.  Yes, I agree.
> 
> > Now, for efficiency sake, I do think we might be able to implement this slightly
> > differently from DTrace v1 without creating different behaviour.  I think it
> > would be valid to first try to find a matching probe first and if one is not
> > found, to then give all providers a chance to provide it.  We could then loop
> > back to performing the lookup one more time.
> 
> It seems to me that such an optimization would change semantics. E.g., 
> let's imagine that these two probes are provided on demand:
>      foo:bar:baz:name1
>      foo:bar:baz:name2
> What would "-n foo:bar:baz:name1 -n foo:bar:baz:*" do?
> 
> With provide-look, the second probe description would find both name1 
> and name2.
> 
> With look-provide-look, the second probe description would see only name1.
> 
> Or am I missing something?

Yes, in case of globs we ought to do provide-lookup, whereas in the absence
of globs we can do lookup-provide-lookup.

> > That is an optimiztion that we could leave for later but it seems to be a very
> > simple and obvious one.  Most of the time, probes people refer to will already
> > exist.  The lookup process is also a lot faster than DTrace v1 could provide
> > (due to the userspace-kernel interaction), so the penalty for
> > lookup-provide-lookup is very small.
> 
> How about we defer discussion of this change (mainly because I'm 
> concerned about the change in semantics).

OK with me.

> >>   	/*
> >>   	 * Special case: if the probe is fully specified (none of the elements
> >>   	 * are empty of a glob pattern, we can do a direct lookup based on the
> >> diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
> >> index e98bfac1..50f663f1 100644
> >> --- a/libdtrace/dt_provider.h
> >> +++ b/libdtrace/dt_provider.h
> >> @@ -62,6 +62,8 @@ typedef struct dt_provimpl {
> >>   	int (*probe_info)(dtrace_hdl_t *dtp,	/* get probe info */
> >>   			  const struct dt_probe *prp,
> >>   			  int *idp, int *argcp, dt_argdesc_t **argvp);
> >> +	int (*provide)(dtrace_hdl_t *dtp,	/* provide probes */
> >> +		       const dtrace_probedesc_t *pdp);
> > I think it would be more appropriate to place this right after the populate
> > callback because they perform the same kind of function (add probes to the
> > global probe list).
> 
> Okay.
> 
> >>   	void (*trampoline)(dt_pcb_t *pcb);	/* generate BPF trampoline */
> >>   	int (*probe_fini)(dtrace_hdl_t *dtp,	/* probe cleanup */
> >>   			  struct dt_probe *prb);
> >> -- 
> >> 2.18.2
> >>
> >>
> >> _______________________________________________
> >> DTrace-devel mailing list
> >> DTrace-devel at oss.oracle.com
> >> https://oss.oracle.com/mailman/listinfo/dtrace-devel
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel