[DTrace-devel] [PATCH 06/13] Add provider-dependent struct in dt_probe_t

[Eugene Loh]

On 07/07/2020 11:02 PM, Kris Van Hees wrote:

> On Tue, Jul 07, 2020 at 10:11:44PM -0700, Eugene Loh wrote:
>> On 07/06/2020 02:50 PM, Kris Van Hees wrote:
>>> On Wed, Jul 01, 2020 at 10:41:11PM -0400, eugene.loh at oracle.com wrote:
>>>> From: Eugene Loh <eugene.loh at oracle.com>
>>>>
>>>> The dt_probe_t struct had members like event_id and event_fd.
>>>> This made sense for the providers we were supporting, since they
>>>> were all tracepoint-like.  For future providers, however, those
>>>> members might not make sense and others will be needed.  Therefore,
>>>> replace event_id and event_fd with an opaque pointer to a
>>>> provider-dependent struct.
>>>>
>>>> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
>>>>
>>>> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
>>>> index be407dd2..496ba11e 100644
>>>> --- a/libdtrace/dt_probe.c
>>>> +++ b/libdtrace/dt_probe.c
>>>> @@ -505,6 +505,7 @@ dt_probe_destroy(dt_probe_t *prp)
>>>>    
>>>>    	if (prp->prov && prp->prov->impl && prp->prov->impl->probe_fini)
>>>>    		prp->prov->impl->probe_fini(dtp, prp);
>>>> +	dt_free(dtp, prp->prv_data);
>>> OK
>>>
>>>>    
>>>>    	dt_node_list_free(&prp->nargs);
>>>>    	dt_node_list_free(&prp->xargs);
>>>> @@ -665,7 +666,8 @@ dt_probe_tag(dt_probe_t *prp, uint_t argn, dt_node_t *dnp)
>>>>    }
>>>>    
>>>>    dt_probe_t *
>>>> -dt_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
>>>> +dt_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const void *prv_data,
>>>> +		const char *prv,
>>>>    		const char *mod, const char *fun, const char *prb)
>>> Please move the prv_data to the end of the argument list, which is more
>>> consistent with other code that supports opaque pointers.  It also shows its
>>> status as auxilliary information rather than being mixed with the elements
>>> that are part of the generic probe.  And perhaps name it datap since that is
>>> the name you used for the provider-specific data variable in other places.
>>>
>>> I would also modify dt_probe_insert() to free datap (if not NULL) if the probe
>>> cannot be created.  That avoids needing to have each caller check if the probe
>>> got created, and if not, free datap.

Maybe this is beside the point but FWIW with all tp providers using the 
new tp_probe_insert(), there are much fewer callers of dt_probe_insert() 
than there were before.  FWIW.

>> I don't think this really works.  The datap points to provider-specific
>> stuff.  It might even point to memory that contains pointers to other
>> things.  So the provider needs to free datap.
>>
>> Another option is that the provider only allocates datap but doesn't set
>> up the memory to which datap points, doing that only once the probe has
>> been inserted.  But then if there is a problem with initializing the
>> memory to which datap points, would we want to delete the probe we just
>> inserted?  We do not yet have a good function for that (deleting one
>> probe, vs cleaning them all up at the end... e.g., never do we decrement
>> dt_probe_id).
>>
>> Anyhow, one can shoehorn the change you're talking about, but it strikes
>> me as pretty ugly.
>>
>>> You already do a freeing of the datap in
>>> dt_probr_destroy() so doing it in dt_probe_insert() as well (when needed) is
>>> OK.
>> Well, no.  I mean, yes, there is a free(datap) in dt_probe_destroy(),
>> but it comes right after calling the provider-specific fini() function.
>> So, do we want dt_probe_insert(), called by a provider, to then call a
>> provider-specific callback we need to define?  My head is spinning just
>> thinking of the back-and-forth.
> If we anticipate that the provider-specific data will contains pointers to
> other memory blocks that would need to be cleaned up along with it (I cannot
> really think of a need for that in any provider we're planning to support but
> it might turn out to be needed)

What about the profile provider?  For profile-n probes, we have 
something on each CPU.  So we end up with an array of file descriptors, 
one per CPU.  datap points to (among other things) a pointer to such an 
array.

> then we will be needing a function to destroy
> the provider-specific data anyhow.

Right.  The probe_fini() function.  Just seems funny to me to have the 
provider call dt_probe_insert(), which calls back to the provider, then 
returns to dt_probe_insert(), and then back to the provider.  
Nevertheless, just calling probe_fini(dtp,prp) here simply doesn't work 
since it takes a prp argument;  meanwhile, the particular case we're 
discussing here is when we are unable to create a prp.

> I think it is safe right now to just have
> dt_probe_insert() do it (similar to dt_probe_destroy())
>
> The fact that the one in dt_probe_destroy() comes after calling a probe_fini()
> (if one exists) is not really a good argument.  You wouldn't want the fini()
> to do the cleanup of stuff referenced from datap and then have datap free'd
> by dt_probe_destroy().

Why wouldn't I?

Actually, let's just assume we won't agree on the woulds and shoulds.  
Let's just figure out some way that works.  I don't care if it seems 
unnecessarily convoluted to me;  if I can implement it, I will.  If I 
understand what you're proposing:

*)  introduce a new provider-specific function ("data_free" maybe?) that 
takes a datap and
frees resources it points to as well as freeing datap itself

*)  have dt_probe_insert() try to form a prp;  if it cannot, it will 
call the new function and then return a NULL prp

*)  change probe_fini() to take advantage of the new function

Do I understand correctly?  If that's what you're proposing, I will 
implement it.

> If you envision more complex datap variants where there
> is additional allocated data, then we really need to have a function to handle
> the freeing of datap and related structures.  And that can then be called from
> dt_probe_insert() if the probe creation fails.
>
> I still think a plan free of datap in dt_probe_insert() suffices for now, and
> we can always implement a function to do the freeing later if it turns out to
> be necessary.  Right now it seems to be a bit of over-engineering.
>
>>> The caller can count on the fact that if the probe could not be created,
>>> datap is no longer valid.