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

[Eugene Loh]

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.

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