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

[Kris Van Hees]

On Thu, Feb 23, 2023 at 01:27:55PM +0000, Nick Alcock wrote:
> 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).

The overlying/underlying mechanism as used for pid and usdt probes based on
uprobes is not useable here and so I don't want to use terminology that sounds
very much like that approach.

Your argument that 'dependent probe' could refer to either underlying or the
one that uses the underlying probe is not valid because in almost all cases
(for SDT providers) the underlying probe most definitely has a reason to
exist (as opposed to the pid/usdt case).

The proc provider (and other SDT providers) make probes available that
*depend* on the existance of another "real" probe (and I refer to those s
underlying probes because the fact that they are the probes that we will
actually attach a BPF program to *and* the fact that other probes will make
use of them) is something they have in common with the pid/usdt case.

A proc probe *depends* on an underlying probe as firing mechanism and it
depends on the underlying probe for its argument information, while needing
(most of the time) some translation of arguments because the signature is
(usually) different.

> > 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?

Yes, because the code path to generate the trampoline would fail otherwise
due to obscure parser/code generator/compiler interactions.

> > +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.)

Again, "dependent" should not really be a confusing concept.  Take it this
way...  Using your terminology, what would the meaning be of the underlying
probe to depend on the overlying probe?  What aspects of the overlying probe
would the underlying probe depend on?  Wouldn't that essentially mean that
the overlying probe can exist without the underlying one (which is obviously
wrong)?

Also note the difference with pid/usdt probes...  There we implemented a uprobe
provider (for underlying probes) because we needed it to make pid and usdt
probes work (the overlying probes).  The only reason for the uprobe provider
to exist is to serve pid and usdt probes.  There are no 'uprobe' probes in
general.  The uprobe provider provides *no* probes at all.

In this case, SDT-provider probes are implemented using probes that already
exist (using any of the various providers that we implemented).  They depend
on those probes to be available, and that is why I call probes in these SDT
providers 'dependent probes'.

I think part of your confusion may stem from the fact that you came up with
the overlying/underlying terminology when you expanded my pid provider
implementation to also support usdt probes.  So that is the context in which
you have been operating...  but this is different.

If this is a major hurdle, I can consider some terminology changing but *not*
to the underlying/overlying terminology.  That wouldn't make sense.

> > @@ -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.

Short answer: we already have a concept of underlying and overlying probes,
and this is clearly *not* using that because it isn't sufficient for what we
need here.  So using overlying/underlying terminology would be confusing since
it implies a mechanism that we are actually not using.
> 
> > +	 *	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.)

Yes, there really isn't a good reason to bother with something more fancy.