[DTrace-devel] [PATCH 5/5 v2] Add a profile provider

[Eugene Loh]

On 06/15/2020 09:48 PM, Kris Van Hees wrote:

> On Mon, Jun 15, 2020 at 02:18:05PM -0400, eugene.loh at oracle.com wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> The probes use perf_events with PERF_TYPE_SOFTWARE and
>> PERF_COUNT_SW_CPU_CLOCK.
>>
>> There are many FIXMEs.  Among them, the trampoline does not
>> yet write the entire DTrace context... at least not the DTrace
>> probe arguments.  Also, profile-n is not yet implemented.
>>
>> For other providers, the work of setting a probe's event_fd is
>> done in bpf_attach(), but the profile provider is sufficiently
>> different that it makes more sense to do so in the provider's
>> probe_info() function.  That is, we would like to set the
>> probe's event_id and event_fd in probe_info().  On the other
> I don't believe this makes sense.  The point is that a probe is not created
> (or enabled) until we are going to use it.

I wanted to get clarification on what you mean.  Currently, we "create" 
probes -- like dtrace's uprobes and fbt's kprobes -- during the 
providers' probe_info() functions.  This occurs during, e.g., 
cmd/dtrace.c's "fourth phase" of processing command-line options -- that 
is, when D is being compiled and before we do any "list" or "exec" stuff 
(i.e., whether or not we're going to use the probes). I assume such 
behavior is inconsistent with the point you're making?

> During the populate and provide
> processing all that really ought to be done is create the probes with their
> relevant (provider specific) data.  Then when we are ready to actually start
> enabling probes and loading programs we ought to make the perf_event_open
> calls to instantiate the kernel level events/probes.
>
> Since the profile provider is quite different from the other existing providers
> (and pid provider will be similarly different) it makes sense to introduce
> another callback function for the provider API:
>
> 	int enable(dtrace_hdl_t *dtp, const dt_probe_t *prp)
>
> This function is called for a specifc probe in the provider, and is expected
> to return a file descriptor (typically return value of perf_event_open()) or
> -1 upon error.  The caller should assign this fd (return value) to the event_fd
> field in the dt_probe_t struct.

I can look at such a function, but I have some questions:

*)  Would this be to replace the event_fd initialization in dt_bpf_attach()?

*)  In fact, wouldn't it make sense to eliminate dt_bpf_attach() 
completely, and just have dt_bpf_stmt() call some 
prp->prov->impl->enable(dtp, prp, fd) to attach the BPF program (fd) to 
the D "probe"?  I bring this up due to the side discussion we were 
having about how support for "profile-n" probes (which are implemented 
with many, per-CPU perf_events) need multiple event_fd's per D probe.  
Specifically, the profile-provider-specific enable (or attach) function 
would not return an event_fd.

> This was the work of actually creating and enabling the underlying kernel perf
> event can be handled by each provider based on its unique requirements.
>
>> hand, the function was declared to take a "const" probe.
>> This didn't even make sense, however, since the function's
>> only caller, dt_probe_args_info(), retrieved information
>> specifically so that it could set fields like event_id.  So,
>> this commit removes the "const" from the probe in probe_info()'s
>> declaration.
> Why didn't it make sense that probe_info() specifies the 'const' modifier on
> the prp argument?  The probe_info() function does not modify any fields in
> prp (dt_probe_t), nor is it expected to.  So 'const' is sensible here.

The only call chain that calls the providers' probe_info() functions is
dt_probe_info()
-> dt_probe_args_info()
-> prp->prov->impl->probe_info()
Not only does this call chain modify the "dt_probe_t *prp", it actually 
creates it from scratch.  So while it was the case that probe_info() 
wasn't itself modifying prp, it was only being used to construct prp.  
It did not seem important to retain that const.  In particular, if the 
profile-provider probe_info() function were used to create the probe -- 
which is what the dtrace and fbt providers were already doing anyhow -- 
then not much other code had to be touched.

But I think I can do your "enable()" suggestion, in which case this 
const business does not need to be addressed.