[DTrace-devel] [PATCH 14/15] cg: Implement dependent probes

[Nick Alcock]

On 23 Feb 2023, Kris Van Hees via DTrace-devel said:

> Some providers may wish to implement probes on top of other probes.
> This patch provides the concept of dependent probes: probes that use
> other probes as their firing mechanism.  This is accomplished by
> generating a custom trampoline that converts the DTrace context of
> the underlying probe into a DTrace context for the dependent probe
> and then adding the clauses for the dependent probe to the underlying
> probe.

YES!

But I don't in general like the word 'dependent' because it's so damn
ambiguous. You could just as well say that the underlying probes depend
on the overlying ones, since without the overlying ones the underlying
ons have no reason to be there.

What's wrong with the old underlying/overlying terminology? At least
that term was easy to remember, while I'm afraid in just the time it's
taken me to write this paragraph I've already forgotten which layer of
probe is meant to be 'dependent' on which (and I'm mentally translating
it to underlying/overlying to make any sense of it).

> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>

Reviewed-by: Nick Alcock <nick.alcock at oracle.com>

with a number of strong requests, notably PLEASE RENAME THEM. :)

> ---
>  libdtrace/dt_cc.c    |  2 ++
>  libdtrace/dt_cg.c    | 46 +++++++++++++++++++++++++++++++++
>  libdtrace/dt_probe.c | 60 +++++++++++++++++++++++++++++++++++++++-----
>  libdtrace/dt_probe.h |  7 ++++++
>  4 files changed, 109 insertions(+), 6 deletions(-)
>
> diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> index bbc203c3..88d7f1fe 100644
> --- a/libdtrace/dt_cc.c
> +++ b/libdtrace/dt_cc.c
> @@ -2119,6 +2119,8 @@ dt_construct(dtrace_hdl_t *dtp, dt_probe_t *prp, uint_t cflags, dt_ident_t *idp)
>  	pcb.pcb_probe = prp;
>  	pcb.pcb_pdesc = prp->desc;
>  
> +	yybegin(YYS_DONE);

... is this hunk actually related?

> +static int
> +dt_cg_add_dependent(dtrace_hdl_t *dtp, dt_probe_t *prp, void *arg)

(see above niggle re naming. I have no real idea which probe this is
adding: the overlying one? ... yes, I think so.)

> @@ -519,6 +549,22 @@ void
>  dt_cg_tramp_epilogue(dt_pcb_t *pcb)
>  {
>  	dt_cg_tramp_call_clauses(pcb, pcb->pcb_probe, DT_ACTIVITY_ACTIVE);
> +	/*
> +	 * For each dependent probe (if any):

So I read through the code, worked out which layer was 'dependent', got
down here, about half a minute later, and realised I'd forgotten it again.

If you stick with this terminology I will be unable to understand this
code in only a day or so :( I am now reviewing it with a sticky note on
the monitor that reads simply "DEPENDENT == OVERLYING" and consulting
the note every twenty seconds or so. I guess I'm getting old and can't
change my terminology at whim like I used to :)

> +	 *	1.1 Call dt_cg_tramp_save_args()
> +	 *	1.2 Set PRID to the probe ID of the dependent probe
> +	 *	1.3 Call prp->prov->impl->trampoline()
> +	 *		[ This will generate the pseudo-trampoline that sets
> +	 *		  up the arguments for the dependent probe, possibly
> +	 *		  based on the arguments of the underllying probe. ]

... presumably this needs your earlier change to trampoline
(non)generation.

> +	 *	1.4 Call dt_cg_tramp_call_clauses() for the dependent probe

... I guess it's called by this instead? Why not have a flag on the
underlying probe, set by overlying probe addition, that says 'this is an
underlying probe' and skip trampoline generation if that is set? (aside:
if you're still calling the underlying probe "underlying", why on earth
not call the other one "overlying"? That's an obvious antonym:
"dependent" really is not.)

I'll stop whining about this now, but assume it applies to all the type
and function names too :) every one of which is forcing me to stop and
look at that sticky note again and reason this naming scheme though from
first principles.

> +	 *	1.1 Call dt_cg_tramp_restore_args()
> +	 *
> +	 * Possible iptimization:

(optimization)

> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> index e50fc7ba..a5952c57 100644
> --- a/libdtrace/dt_probe.c
> +++ b/libdtrace/dt_probe.c
> @@ -24,10 +24,15 @@
>  #include <dt_list.h>
>  #include <dt_bpf.h>
>  
> -typedef struct dt_probeclause {
> +typedef struct dt_probe_clause {

YES!

> +int
> +dt_probe_add_dependent(dtrace_hdl_t *dtp, dt_probe_t *prp, dt_probe_t *dprp)
> +{
> +	dt_probe_dependent_t	*pdp;
> +
> +	/* Ignore dependent probes already in the list. */
> +	for (pdp = dt_list_next(&prp->dependents); pdp != NULL;
> +	     pdp = dt_list_next(pdp)) {
> +		if (pdp->probe == dprp)
> +			return 0;
> +	}

(the list is usually very short, presumably, since this is O(n^2) in the
number of dependent probes.)