Re: [DTrace-devel] [PATCH v3 dtrace 1/4] dt_provider_tp: add optional event data, freed on tp free

[Kris Van Hees <kris.van.hees@oracle.com>]

I don't think this is the right approach.  At least, with other code (uprobe
provider) we went the route of having the provider declare its own custom
struct to store provider-specific probe info along with a pointer to the
tp_probe_t (tracepoint probe info).  I think we should do the same here.

I'm preparing a modified patch for this as part of a small series to fix a
few design issues with function naming in dt_provider_tp, to show what I mean.

I'll continue reviewing the rest of this series based on that upcomming change.

E.g. here we can use:
struct fbt_probe {
	char		*tp_name;
	tp_probe_t	*tp;
} fbt_probe_t;

and an fbt_probe_t instance will be passed to dt_probe_insert( and thus will
be available as prp->prv_data.  Whenevef this provider needs to do anything
with an FBT probe's underlying tracepoint, it can be done by calling the
appropriate dt_tp_* function and passing in ((fbt_probe_t *prp->prv_data)->tp.

Anyway, series coming (hopefully) tomorrow.

On Wed, Oct 16, 2024 at 04:54:06PM +0100, Alan Maguire via DTrace-devel wrote:
> It will be used to store suffixed function name for kprobe attach to
> "foo.isra.0"-style functions.  Such functions use the unsuffixed form
> as probe name, so we need to store the full function name to support
> attach.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  libdtrace/dt_provider_tp.c | 27 +++++++++++++++++++++++++++
>  libdtrace/dt_provider_tp.h |  8 ++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
> index 50df2328..fc3c216a 100644
> --- a/libdtrace/dt_provider_tp.c
> +++ b/libdtrace/dt_provider_tp.c
> @@ -33,6 +33,7 @@
>  struct tp_probe {
>  	uint32_t	event_id;	/* tracepoint event id or BTF id */
>  	int		event_fd;	/* tracepoint perf event fd */
> +	void		*event_data;	/* optional data */
>  };
>  
>  /*
> @@ -313,6 +314,8 @@ dt_tp_detach(dtrace_hdl_t *dtp, tp_probe_t *tpp)
>  void
>  dt_tp_destroy(dtrace_hdl_t *dtp, tp_probe_t *tpp)
>  {
> +	if (tpp && tpp->event_data)
> +		free(tpp->event_data);
>  	dt_free(dtp, tpp);
>  }
>  
> @@ -334,6 +337,30 @@ dt_tp_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
>  	return dt_probe_insert(dtp, prov, prv, mod, fun, prb, tpp);
>  }
>  
> +dt_probe_t *
> +dt_tp_probe_insert_data(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
> +			const char *mod, const char *fun, const char *prb,
> +			void *data)
> +{
> +	tp_probe_t	*tpp;
> +
> +	dt_probe_t *prp = dt_tp_probe_insert(dtp, prov, prv, mod, fun, prb);
> +
> +	if (prp) {
> +		tpp = prp->prv_data;
> +		tpp->event_data = data;
> +	}
> +	return prp;
> +}
> +
> +void *
> +dt_tp_get_event_data(const dt_probe_t *prp)
> +{
> +	tp_probe_t	*tpp = prp->prv_data;
> +
> +	return tpp->event_data;
> +}
> +
>  uint32_t
>  dt_tp_get_event_id(const dt_probe_t *prp)
>  {
> diff --git a/libdtrace/dt_provider_tp.h b/libdtrace/dt_provider_tp.h
> index e7d52876..66a9bd80 100644
> --- a/libdtrace/dt_provider_tp.h
> +++ b/libdtrace/dt_provider_tp.h
> @@ -41,6 +41,14 @@ extern struct dt_probe *dt_tp_probe_insert(dtrace_hdl_t *dtp,
>  					   dt_provider_t *prov,
>  					   const char *prv, const char *mod,
>  					   const char *fun, const char *prb);
> +extern struct dt_probe *dt_tp_probe_insert_data(dtrace_hdl_t *dtp,
> +						dt_provider_t *prov,
> +						const char *prv,
> +						const char *mod,
> +						const char *fun,
> +						const char *prb,
> +						void *data);
> +extern void *dt_tp_get_event_data(const struct dt_probe *prp);
>  extern uint32_t dt_tp_get_event_id(const struct dt_probe *prp);
>  extern void dt_tp_set_event_id(const struct dt_probe *prp, uint32_t id);
>  extern int dt_tp_probe_info(dtrace_hdl_t *dtp, FILE *f, int skip,
> -- 
> 2.43.5
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel