[DTrace-devel] [PATCH v2] provider: support (opaque) private data for providers
Kris Van Hees
kris.van.hees at oracle.com
Fri May 19 02:34:38 UTC 2023
On Thu, May 18, 2023 at 06:56:50PM -0400, Eugene Loh via DTrace-devel wrote:
> I don't 100% understand, but:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
>
> I'm curious, though. The CPC provider has this comment:
> * FIXME: Memory pointed to by next_probe_map,
> pfmname, and Dname
> * should ideally be freed explicitly during some
> probe_destroy(),
> * but this is a low priority since all such memory
> will be freed
> * anyhow when the DTrace session ends.
> Should there be a plan to address this FIXME? Shall I compose a follow-up
> patch? Or, continue to defer to "the future"?
It should be pretty easy to implement a 'destroy' callback function for this,
so I think it is worth doing now.
> On 5/18/23 15:27, Kris Van Hees via DTrace-devel wrote:
>
> > Commit 2ba7c3bd ("Add a CPC provider") added an opaque private data
> > pointer to the provider implementation struct. In retrospect, that
> > does not seem to be the best place because the implementation struct
> > is really meant for callback function pointers. The provider struct
> > itself is a better place for this. SDT providers will also make use
> > of the ability to store private data for providers.
> >
> > Signed-off-by: Kris Van Hees<kris.van.hees at oracle.com>
> > ---
> > libdtrace/dt_parser.c | 5 +++--
> > libdtrace/dt_prov_cpc.c | 45 ++++++++++++++++++++++---------------
> > libdtrace/dt_prov_dtrace.c | 2 +-
> > libdtrace/dt_prov_fbt.c | 2 +-
> > libdtrace/dt_prov_proc.c | 2 +-
> > libdtrace/dt_prov_profile.c | 2 +-
> > libdtrace/dt_prov_rawtp.c | 2 +-
> > libdtrace/dt_prov_sched.c | 2 +-
> > libdtrace/dt_prov_sdt.c | 2 +-
> > libdtrace/dt_prov_syscall.c | 2 +-
> > libdtrace/dt_prov_uprobe.c | 10 ++++-----
> > libdtrace/dt_provider.c | 9 ++++++--
> > libdtrace/dt_provider.h | 8 ++++---
> > 13 files changed, 55 insertions(+), 38 deletions(-)
> >
> > diff --git a/libdtrace/dt_parser.c b/libdtrace/dt_parser.c
> > index ea9ff15b..4f81b1c5 100644
> > --- a/libdtrace/dt_parser.c
> > +++ b/libdtrace/dt_parser.c
> > @@ -1,6 +1,6 @@
> > /*
> > * Oracle Linux DTrace.
> > - * Copyright (c) 2006, 2022, Oracle and/or its affiliates. All rights reserved.
> > + * Copyright (c) 2006, 2023, Oracle and/or its affiliates. All rights reserved.
> > * Licensed under the Universal Permissive License v 1.0 as shown at
> > *http://oss.oracle.com/licenses/upl.
> > */
> > @@ -2644,7 +2644,8 @@ dt_node_provider(char *name, dt_node_t *probes)
> > if ((dnp->dn_provider = dt_provider_lookup(dtp, name)) != NULL)
> > dnp->dn_provred = B_TRUE;
> > else if (!(dnp->dn_provider = dt_provider_create(dtp, name, NULL,
> > - &_dtrace_prvdesc)))
> > + &_dtrace_prvdesc,
> > + NULL)))
> > longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
> > else
> > dnp->dn_provider->pv_flags |= DT_PROVIDER_INTF;
> > diff --git a/libdtrace/dt_prov_cpc.c b/libdtrace/dt_prov_cpc.c
> > index aec4367d..d65ba6b6 100644
> > --- a/libdtrace/dt_prov_cpc.c
> > +++ b/libdtrace/dt_prov_cpc.c
> > @@ -50,16 +50,12 @@ typedef struct cpc_probe_map {
> > char *pfmname;
> > } cpc_probe_map_t;
> > -static dt_probe_t *cpc_probe_insert(dtrace_hdl_t *dtp, const char *prb)
> > +static dt_probe_t *cpc_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prv,
> > + const char *prb)
> > {
> > - dt_provider_t *prv;
> > cpc_probe_t *datap;
> > int i, cnt = dtp->dt_conf.num_online_cpus;
> > - prv = dt_provider_lookup(dtp, prvname);
> > - if (!prv)
> > - return 0;
> > -
> > datap = dt_zalloc(dtp, sizeof(cpc_probe_t));
> > if (datap == NULL)
> > return NULL;
> > @@ -82,9 +78,13 @@ err:
> > static int populate(dtrace_hdl_t *dtp)
> > {
> > int n = 0;
> > + dt_list_t *listp = dt_zalloc(dtp, sizeof(dt_list_t));
> > + dt_provider_t *prv;
> > - dt_provider_create(dtp, prvname, &dt_cpc, &pattr);
> > - dt_cpc.prv_data = dt_zalloc(dtp, sizeof(dt_list_t));
> > + if (listp == NULL)
> > + return 0;
> > +
> > + prv = dt_provider_create(dtp, prvname, &dt_cpc, &pattr, listp);
> > /* incidentally, pfm_strerror(pfm_initialize()) describes the error */
> > if (pfm_initialize() != PFM_SUCCESS)
> > @@ -215,7 +215,7 @@ static int populate(dtrace_hdl_t *dtp)
> > continue;
> > for (unsigned char *p = next_probe_map->Dname; *p; p++)
> > *p = (*p == '-') ? '_' : tolower(*p);
> > - dt_list_append(dt_cpc.prv_data, next_probe_map);
> > + dt_list_append(listp, next_probe_map);
> > /*
> > * Compose a CPC probe name by adding mode "all" and a sample period
> > @@ -234,7 +234,8 @@ static int populate(dtrace_hdl_t *dtp)
> > pd.mod = modname;
> > pd.fun = funname;
> > pd.prb = s;
> > - if (dt_probe_lookup(dtp, &pd) == NULL && cpc_probe_insert(dtp, s))
> > + if (dt_probe_lookup(dtp, &pd) == NULL &&
> > + cpc_probe_insert(dtp, prv, s))
> > n++;
> > dt_free(dtp, s);
> > @@ -244,15 +245,17 @@ static int populate(dtrace_hdl_t *dtp)
> > return n;
> > }
> > -static int decode_event(struct perf_event_attr *ap, const char *name) {
> > +static int decode_event(struct perf_event_attr *ap, dt_provider_t *prv,
> > + const char *name) {
> > cpc_probe_map_t *probe_map;
> > pfm_perf_encode_arg_t encoding;
> > /* find the probe name mapping for this D name */
> > - for (probe_map = dt_list_next(dt_cpc.prv_data);
> > - probe_map; probe_map = dt_list_next(probe_map))
> > + for (probe_map = dt_list_next(prv->prv_data); probe_map;
> > + probe_map = dt_list_next(probe_map))
> > if (strcmp(name, probe_map->Dname) == 0)
> > break;
> > +
> > if (probe_map == NULL)
> > return -1;
> > @@ -293,7 +296,8 @@ static int decode_attributes(struct perf_event_attr *ap, const char *name) {
> > return -1;
> > }
> > -static int decode_probename(struct perf_event_attr *ap, const char *name) {
> > +static int decode_probename(struct perf_event_attr *ap, dt_provider_t *prv,
> > + const char *name) {
> > char buf[DTRACE_NAMELEN];
> > char *pend;
> > @@ -307,7 +311,7 @@ static int decode_probename(struct perf_event_attr *ap, const char *name) {
> > return -1;
> > *pend = '\0';
> > pend++;
> > - if (decode_event(ap, name) < 0)
> > + if (decode_event(ap, prv, name) < 0)
> > return -1;
> > /* "mode" substring */
> > @@ -342,8 +346,13 @@ static int decode_probename(struct perf_event_attr *ap, const char *name) {
> > static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
> > {
> > + dt_provider_t *prv;
> > struct perf_event_attr attr;
> > + prv = dt_provider_lookup(dtp, prvname);
> > + if (!prv)
> > + return 0;
> > +
> > /* make sure we have IDNONE and a legal name */
> > if (pdp->id != DTRACE_IDNONE || strcmp(pdp->prv, prvname) ||
> > strcmp(pdp->mod, modname) || strcmp(pdp->fun, funname))
> > @@ -354,11 +363,11 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
> > return 0;
> > /* check if the probe name can be decoded */
> > - if (decode_probename(&attr, pdp->prb) == -1)
> > + if (decode_probename(&attr, prv, pdp->prb) == -1)
> > return 0;
> > /* try to add this probe */
> > - if (cpc_probe_insert(dtp, pdp->prb) == NULL)
> > + if (cpc_probe_insert(dtp, prv, pdp->prb) == NULL)
> > return 0;
> > return 1;
> > @@ -419,7 +428,7 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> > char *name = datap->name; /* same as prp->desc->prb */
> > memset(&attr, 0, sizeof(attr));
> > - if (decode_probename(&attr, name) < 0)
> > + if (decode_probename(&attr, prp->prov, name) < 0)
> > return -1;
> > attr.wakeup_events = 1;
> > diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
> > index 939ab1cc..0c3bb135 100644
> > --- a/libdtrace/dt_prov_dtrace.c
> > +++ b/libdtrace/dt_prov_dtrace.c
> > @@ -40,7 +40,7 @@ static int populate(dtrace_hdl_t *dtp)
> > dt_probe_t *prp;
> > int n = 0;
> > - prv = dt_provider_create(dtp, prvname, &dt_dtrace, &pattr);
> > + prv = dt_provider_create(dtp, prvname, &dt_dtrace, &pattr, NULL);
> > if (prv == NULL)
> > return 0;
> > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > index dd6944e2..d08ec830 100644
> > --- a/libdtrace/dt_prov_fbt.c
> > +++ b/libdtrace/dt_prov_fbt.c
> > @@ -69,7 +69,7 @@ static int populate(dtrace_hdl_t *dtp)
> > dtrace_syminfo_t sip;
> > dtrace_probedesc_t pd;
> > - prv = dt_provider_create(dtp, prvname, &dt_fbt, &pattr);
> > + prv = dt_provider_create(dtp, prvname, &dt_fbt, &pattr, NULL);
> > if (prv == NULL)
> > return 0;
> > diff --git a/libdtrace/dt_prov_proc.c b/libdtrace/dt_prov_proc.c
> > index fefe6116..42698e3a 100644
> > --- a/libdtrace/dt_prov_proc.c
> > +++ b/libdtrace/dt_prov_proc.c
> > @@ -121,7 +121,7 @@ static int populate(dtrace_hdl_t *dtp)
> > int i;
> > int n = 0;
> > - prv = dt_provider_create(dtp, prvname, &dt_proc, &pattr);
> > + prv = dt_provider_create(dtp, prvname, &dt_proc, &pattr, NULL);
> > if (prv == NULL)
> > return 0;
> > diff --git a/libdtrace/dt_prov_profile.c b/libdtrace/dt_prov_profile.c
> > index 22dcca5c..476dea7c 100644
> > --- a/libdtrace/dt_prov_profile.c
> > +++ b/libdtrace/dt_prov_profile.c
> > @@ -76,7 +76,7 @@ static int populate(dtrace_hdl_t *dtp)
> > int profile_n[] = { 97, 199, 499, 997, 1999, 4001, 4999 };
> > int tick_n[] = { 1, 10, 100, 500, 1000, 5000 };
> > - prv = dt_provider_create(dtp, prvname, &dt_profile, &pattr);
> > + prv = dt_provider_create(dtp, prvname, &dt_profile, &pattr, NULL);
> > if (prv == NULL)
> > return 0;
> > diff --git a/libdtrace/dt_prov_rawtp.c b/libdtrace/dt_prov_rawtp.c
> > index 9148e715..1ff0a311 100644
> > --- a/libdtrace/dt_prov_rawtp.c
> > +++ b/libdtrace/dt_prov_rawtp.c
> > @@ -71,7 +71,7 @@ static int populate(dtrace_hdl_t *dtp)
> > char *p;
> > size_t n;
> > - prv = dt_provider_create(dtp, prvname, &dt_rawtp, &pattr);
> > + prv = dt_provider_create(dtp, prvname, &dt_rawtp, &pattr, NULL);
> > if (prv == NULL)
> > return 0;
> > diff --git a/libdtrace/dt_prov_sched.c b/libdtrace/dt_prov_sched.c
> > index 145f3bd2..0f51e7c6 100644
> > --- a/libdtrace/dt_prov_sched.c
> > +++ b/libdtrace/dt_prov_sched.c
> > @@ -108,7 +108,7 @@ static int populate(dtrace_hdl_t *dtp)
> > int i;
> > int n = 0;
> > - prv = dt_provider_create(dtp, prvname, &dt_sched, &pattr);
> > + prv = dt_provider_create(dtp, prvname, &dt_sched, &pattr, NULL);
> > if (prv == NULL)
> > return 0;
> > diff --git a/libdtrace/dt_prov_sdt.c b/libdtrace/dt_prov_sdt.c
> > index 0bbe0fa8..8690fc47 100644
> > --- a/libdtrace/dt_prov_sdt.c
> > +++ b/libdtrace/dt_prov_sdt.c
> > @@ -68,7 +68,7 @@ static int populate(dtrace_hdl_t *dtp)
> > char *p;
> > size_t n;
> > - prv = dt_provider_create(dtp, prvname, &dt_sdt, &pattr);
> > + prv = dt_provider_create(dtp, prvname, &dt_sdt, &pattr, NULL);
> > if (prv == NULL)
> > return 0;
> > diff --git a/libdtrace/dt_prov_syscall.c b/libdtrace/dt_prov_syscall.c
> > index ff07d671..d2f2aac1 100644
> > --- a/libdtrace/dt_prov_syscall.c
> > +++ b/libdtrace/dt_prov_syscall.c
> > @@ -75,7 +75,7 @@ static int populate(dtrace_hdl_t *dtp)
> > char *buf = NULL;
> > size_t n;
> > - prv = dt_provider_create(dtp, prvname, &dt_syscall, &pattr);
> > + prv = dt_provider_create(dtp, prvname, &dt_syscall, &pattr, NULL);
> > if (prv == NULL)
> > return 0;
> > diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> > index 33a2db81..f5b724be 100644
> > --- a/libdtrace/dt_prov_uprobe.c
> > +++ b/libdtrace/dt_prov_uprobe.c
> > @@ -62,11 +62,11 @@ dt_provimpl_t dt_usdt;
> > static int populate(dtrace_hdl_t *dtp)
> > {
> > - dt_provider_create(dtp, dt_uprobe.name, &dt_uprobe, &pattr);
> > + dt_provider_create(dtp, dt_uprobe.name, &dt_uprobe, &pattr, NULL);
> > dt_provider_create(dtp, dt_uprobe_is_enabled.name,
> > - &dt_uprobe_is_enabled, &pattr);
> > - dt_provider_create(dtp, dt_pid.name, &dt_pid, &pattr);
> > - dt_provider_create(dtp, dt_usdt.name, &dt_usdt, &pattr);
> > + &dt_uprobe_is_enabled, &pattr, NULL);
> > + dt_provider_create(dtp, dt_pid.name, &dt_pid, &pattr, NULL);
> > + dt_provider_create(dtp, dt_usdt.name, &dt_usdt, &pattr, NULL);
> > return 0;
> > }
> > @@ -231,7 +231,7 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
> > /* Get (or create) the provider for the PID of the probe. */
> > pvp = dt_provider_lookup(dtp, prv);
> > if (pvp == NULL) {
> > - pvp = dt_provider_create(dtp, prv, pvops, &pattr);
> > + pvp = dt_provider_create(dtp, prv, pvops, &pattr, NULL);
> > if (pvp == NULL)
> > return -1;
> > }
> > diff --git a/libdtrace/dt_provider.c b/libdtrace/dt_provider.c
> > index b18ffb50..b601c31a 100644
> > --- a/libdtrace/dt_provider.c
> > +++ b/libdtrace/dt_provider.c
> > @@ -1,6 +1,6 @@
> > /*
> > * Oracle Linux DTrace.
> > - * Copyright (c) 2006, 2021, Oracle and/or its affiliates. All rights reserved.
> > + * Copyright (c) 2006, 2023, Oracle and/or its affiliates. All rights reserved.
> > * Licensed under the Universal Permissive License v 1.0 as shown at
> > *http://oss.oracle.com/licenses/upl.
> > */
> > @@ -45,6 +45,9 @@ dt_provider_del_prov(dt_provider_t *head, dt_provider_t *pvp)
> > if (pvp->pv_probes != NULL)
> > dt_idhash_destroy(pvp->pv_probes);
> > + if (pvp->impl && pvp->impl->destroy)
> > + pvp->impl->destroy(pvp->pv_hdl, pvp->prv_data);
> > +
> > dt_node_link_free(&pvp->pv_nodes);
> > free(pvp->pv_xrefs);
> > free(pvp);
> > @@ -91,7 +94,8 @@ dt_provider_lookup(dtrace_hdl_t *dtp, const char *name)
> > dt_provider_t *
> > dt_provider_create(dtrace_hdl_t *dtp, const char *name,
> > - const dt_provimpl_t *impl, const dtrace_pattr_t *pattr)
> > + const dt_provimpl_t *impl, const dtrace_pattr_t *pattr,
> > + void *datap)
> > {
> > dt_provider_t *pvp;
> > @@ -103,6 +107,7 @@ dt_provider_create(dtrace_hdl_t *dtp, const char *name,
> > pvp->pv_probes = dt_idhash_create(pvp->desc.dtvd_name, NULL, 0, 0);
> > pvp->pv_gen = dtp->dt_gen;
> > pvp->pv_hdl = dtp;
> > + pvp->prv_data = datap;
> > dt_dprintf("creating provider %s\n", name);
> > if (pvp->pv_probes == NULL) {
> > diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
> > index 1721cc7b..252a73d4 100644
> > --- a/libdtrace/dt_provider.h
> > +++ b/libdtrace/dt_provider.h
> > @@ -60,9 +60,10 @@ typedef struct dt_provimpl {
> > int *argcp, dt_argdesc_t **argvp);
> > void (*detach)(dtrace_hdl_t *dtp, /* probe cleanup */
> > const struct dt_probe *prb);
> > - void (*probe_destroy)(dtrace_hdl_t *dtp, /* free provider data */
> > + void (*probe_destroy)(dtrace_hdl_t *dtp, /* free probe data */
> > + void *datap);
> > + void (*destroy)(dtrace_hdl_t *dtp, /* free provider data */
> > void *datap);
> > - void *prv_data; /* provider-specific data */
> > } dt_provimpl_t;
> > /* list dt_dtrace first */
> > @@ -89,6 +90,7 @@ typedef struct dt_provider {
> > ulong_t pv_gen; /* generation # that created me */
> > dtrace_hdl_t *pv_hdl; /* pointer to containing dtrace_hdl */
> > uint_t pv_flags; /* flags (see below) */
> > + void *prv_data; /* provider-specific data */
> > } dt_provider_t;
> > #define DT_PROVIDER_INTF 0x1 /* provider interface declaration */
> > @@ -98,7 +100,7 @@ typedef struct dt_provider {
> > extern dt_provider_t *dt_provider_lookup(dtrace_hdl_t *, const char *);
> > extern dt_provider_t *dt_provider_create(dtrace_hdl_t *, const char *,
> > const dt_provimpl_t *,
> > - const dtrace_pattr_t *);
> > + const dtrace_pattr_t *, void *);
> > extern int dt_provider_xref(dtrace_hdl_t *, dt_provider_t *, id_t);
> > #ifdef __cplusplus
> > -- 2.40.1 _______________________________________________ DTrace-devel
> > mailing list DTrace-devel at oss.oracle.com
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list