[DTrace-devel] [PATCH v2] provider: support (opaque) private data for providers
Eugene Loh
eugene.loh at oracle.com
Thu May 18 22:56:50 UTC 2023
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"?
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
More information about the DTrace-devel
mailing list