[DTrace-devel] [PATCH v3 02/19] Add a dt_provider_discover() function

[Eugene Loh]

On 10/23/24 18:22, Kris Van Hees wrote:

> On Wed, Oct 16, 2024 at 12:01:36PM -0400, eugene.loh at oracle.com wrote:
>> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
>> @@ -1275,6 +1275,12 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
>>   		int		fd;
>>   		int		rc = -1;
>>   
>> +		if (prp->prov->impl->prog_type == BPF_PROG_TYPE_UNSPEC) {
>> +			if (prp->prov->impl->add_probe)
>> +				prp->prov->impl->add_probe(dtp, prp);
>> +			continue;
> This could benefit from a comment perhaps?  It took me a bit to figure out
> why we would want this here, since this is the case where the probes are
> already known to exist (they were discovered during program compilation).
> So, needing a call to add_probe seems out-of-place here.
>
> I do understand (after digging through the code a bit) that it is needed for
> USDT because you add probe name data to the usdt_names BPF map and probe data
> to the usdt_prids BPF map from the add_probe() callback.  But thatis far from
> obvious...  But try to keep the comment non-USDT specific if you can.  Perhaps
> just mention this is needed to handle probe types that are discoverable at a
> later time, because they may need additional processing.

How about:
                 /* Handle probes discovered after compilation. */


> I think that testing for prog_type == UNSPEC is not appropriate here.  That
> seems to be a case of leaking USDT-specific implementation detail into this
> generic function.

It's simply a construct that's lifted from dt_bpf_make_progs().

> One way to deal with this might be to add a test in
> add_probe_uprobe() on dtp->dt_active.  If dt_active == 0, then
> add_probe_uprobe() should immediately return (because the probe already exists
> in the system).  If dt_active == 1, then it is a runtime discovered probe.

I don't understand how that's clearer and even having trouble convincing 
myself this is right.  Also, the dt_active stuff would belong in the 
succeeding patch, already reviewed.  Anyhow, I tried the change and it 
tests fine.  So, I'll send out a next version of this patch, along with 
a new version of the succeeding patch (which had already been reviewed).

>> +		}
>> +
>>   		dp = prp->difo;
>>   		if (dp == NULL)
>>   			continue;