[DTrace-devel] [PATCH 19/38] Retain probe descriptions

[Kris Van Hees]

On Thu, Jul 18, 2024 at 11:34:05PM -0400, Eugene Loh wrote:
> What do you think about replacing the array dt_retained[n] (as in,
> "dt_clause_$n") with a hash table that uses a string (like
> "dt_clause_$n") to look up a probe description?
> 
> On 7/18/24 17:25, Kris Van Hees wrote:
> 
> > I think this needs an actual description of what you are trying to do,
> 
> An underlying probe gets a list of clauses to call in its trampoline. 
> Actually, the underlying probe looks up a mask in some BPF map and then
> walks the clauses, calling them conditionally based on the mask bits.
> 
> To construct the mask for a particular overlying probe, we walk the
> clauses.  Each clause is associated with a particular probe description.  We
> compare the overlying probe to the respective probe description to decide
> whether to set that particular bit.
> 
> So, a uprp has a list of clauses.  We want some way of getting from clauses
> to probe descriptions.  The linear array using that $n was one way.  I can
> move over to the hash table based on clause string name if you prefer.
> 
> (Incidentally, a uprp's clause list is set during dt_probe_add_clause(), at
> which point we do not know the motivating probe description.

Well, yes and no.  At the point of compilation, when there is not a single
task yet with the USDT probes we're wanting to probe, there won't be any
underlying probe(s) yet.  So all there will be are compiled clauses, and the
statements they belong to know the probedesc through the ecbdesc.  So, we
cannot even count on the BPF program already existing with all clauses linked
into it so all that needs to be done is setting the mask.

I am not clear how this fits in the design you came up with.

Even for the case where we already have the underlying probe (and thus the BPF
prgroam), I think it might perhaps be a cleaner design to not just store
clauses in the clause list, but perhaps store statements (which gives access
to the pdescs).  Then, when we encounter a wildcard match that relates us to
an underlying probe that exists, you can just go through the statement list
for that underlying probe and use a running index in that list to know which
bits to set in the bitmap.

> > because
> > retaining pdescs in and of itself does not seem meaningful.  They do not
> > contain a reference to the clause they were associated with when the code was
> > parsed and compiled) and it seems to me that you are doing a sort-of reverse
> > association, and are actually (indirectly) retaining clauses by keeping a
> > link of pdescs, one per clause.  And you identify the clause by its numeric
> > id as obtained from its identifier name (which is dynamically assigned during
> > compilation).  That seems fragile and rather non-intuitive.
> > 
> > If memory serves me right, the intent was to retain enablings.  Of course, in
> > the current implementation of DTrace, enablings are nothing more than probes
> > because the probes have BPF programs associated with them (unless they are
> > overlying probes, in which case the actual code that gets loaded is part of the
> > BPF program of the underlying probe (which will be in the enablings list).
> > 
> > When we are retaining enablings, it seems to me that it would make more sense
> > to keep a list of objects { pdesc, clause } where clause would be the
> > identifier name of the compiled clause (similar to a function name).  Using a
> > pointer to the actual clause instead is (I think) not going to work becauase
> > AFAIK the clause does not store a pointer to its own identiifier.
> > 
> > In the end, this will largely do the same thing as your code (here and when it
> > gets used later), but it removes the implied dependency on how identifier names
> > for clauses are generated.  That could change in the future, and doing so would
> > completely break this imlementation.
> > 
> > On Thu, Jun 27, 2024 at 01:38:10AM -0400, eugene.loh at oracle.com wrote:
> > > From: Eugene Loh <eugene.loh at oracle.com>
> > > 
> > > To support matching probes after dtrace has started, we retain
> > > probe descriptions.
> > > 
> > > Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> > > ---
> > >   libdtrace/dt_impl.h    |  2 ++
> > >   libdtrace/dt_program.c | 37 ++++++++++++++++++++++++++++++++++++-
> > >   2 files changed, 38 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> > > index 2132eda2..445cd602 100644
> > > --- a/libdtrace/dt_impl.h
> > > +++ b/libdtrace/dt_impl.h
> > > @@ -300,6 +300,8 @@ struct dtrace_hdl {
> > >   	dt_list_t dt_programs;	/* linked list of dtrace_prog_t's */
> > >   	dt_list_t dt_xlators;	/* linked list of dt_xlator_t's */
> > >   	dt_list_t dt_enablings;	/* list of (to be) enabled probes */
> > > +	dtrace_probedesc_t **dt_retained; /* array of retained pdescs */
> > > +	int dt_maxretained;	/* number of retained pdescs allocated */
> > >   	struct dt_xlator **dt_xlatormap; /* dt_xlator_t's indexed by dx_id */
> > >   	id_t dt_xlatorid;	/* next dt_xlator_t id to assign */
> > >   	dt_ident_t *dt_externs;	/* linked list of external symbol identifiers */
> > > diff --git a/libdtrace/dt_program.c b/libdtrace/dt_program.c
> > > index a4b052fc..7106c249 100644
> > > --- a/libdtrace/dt_program.c
> > > +++ b/libdtrace/dt_program.c
> > > @@ -163,7 +163,42 @@ dt_prog_stmt(dtrace_hdl_t *dtp, dtrace_prog_t *pgp, dtrace_stmtdesc_t *sdp,
> > >   {
> > >   	pi_state_t		st;
> > >   	dtrace_probedesc_t	*pdp = &sdp->dtsd_ecbdesc->dted_probe;
> > > -	int			rc;
> > > +	char			*sclause = sdp->dtsd_clause->di_name;
> > > +	int			rc, nclause;
> > > +
> > > +	/* Get the clause number from the clause name "dt_clause_"$n. */
> > > +	assert(strncmp(sclause, "dt_clause_", strlen("dt_clause_")) == 0);    // FIXME can probably drop this check (make sure the name starts "dt_clause_")
> > > +	nclause = atoi(sclause + strlen("dt_clause_"));
> > > +	assert(nclause < 65536);	// FIXME are we okay here with some arbitrary limit?
> > > +
> > > +	/* Grow the list of retained pdescs, if necessary. */
> > > +	if (nclause >= dtp->dt_maxretained) {
> > > +		int			newm;
> > > +		dtrace_probedesc_t	**newr;
> > > +
> > > +		/* Figure how big to make the array. */
> > > +		if (dtp->dt_maxretained > 0)
> > > +			newm = 2 * dtp->dt_maxretained;
> > > +		else
> > > +			newm = 8;
> > > +		while (newm <= nclause)
> > > +			newm *= 2;
> > > +
> > > +		/* Allocate the bigger array. */
> > > +		newr = dt_zalloc(dtp, newm * sizeof(dtrace_probedesc_t *));
> > > +		if (newr == NULL)
> > > +			return 0;
> > > +
> > > +		/* Copy data and reset the parameters. */
> > > +		memcpy(newr, dtp->dt_retained, dtp->dt_maxretained * sizeof(dtrace_probedesc_t *));
> > > +		dt_free(dtp, dtp->dt_retained);
> > > +		dtp->dt_retained = newr;
> > > +		dtp->dt_maxretained = newm;
> > > +	}
> > > +
> > > +	/* Add probe description to retained pdescs. */
> > > +	assert(dtp->dt_retained[nclause] == NULL);  // FIXME can probably drop this
> > > +	dtp->dt_retained[nclause] = pdp;        // FIXME I think just pointing to pdp is okay.  No need to cache my own copy.
> > >   	st.cnt = cnt;
> > >   	st.idp = sdp->dtsd_clause;
> > > -- 
> > > 2.18.4
> > >