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

[Kris Van Hees]

On Wed, Jul 08, 2020 at 10:38:18AM -0700, Eugene Loh wrote:
> 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.

That is ok.  Number of callers isn't really relevant here - even if there was
just one caller, dt_probe_insert() would need to exist to do the work of
creating a new probe and inserting it into the hashtables.

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

The probe_fini() function performs the cleanup of the provider specific data,
incuding freeing the fds array, and then dt_probe_destroy() can still do the
free of the datap.

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

Indeed, you would need a function that performs the datap cleanup, and that
gets called from dt_probe_insert() when the probe cannot be created and from
probe_fini() when the probe is being destroyed.

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

probe_destroy(dtrace_hdl_t *dtp, void *datap)

> *)  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.

Yes.

> > 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.
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel