[DTrace-devel] [PATCH 2/7] htab reduction: providers
Eugene Loh
eugene.loh at oracle.com
Thu Oct 7 18:59:43 PDT 2021
A nit: in dt_provider_lookup(), these function names should be flush
against their parentheses:
strlen (name)
sizeof (tmpl.desc.dtvd_name)
strcpy (tmpl.desc.dtvd_name, name)
For stylistic purposes, DTrace-on-BPF applies this rule to sizeof().
See CODING-STYLE.
Otherwise, I have the same high-level comments as before:
*) Instead of maintaining dtp->dt_provlist, should we just have dt_htab
provide an iterator?
*) Instead of maintaining dtp->dt_nprovs, how about destroying the htab
with an iterator or with a function that calls back a destructor?
If those high-level suggestions are rejected, then Reviewed-by: Eugene
Loh <eugene.loh at oracle.com> .
On 10/5/21 9:38 AM, Nick Alcock wrote:
> The list of providers is a hand-rolled fixed-size htab which is grossly
> oversized except when USDT or the pid provider is in use, when it can
> suddenly become terribly undersized: replace it with a dt_htab.
> ---
> libdtrace/dt_impl.h | 3 +-
> libdtrace/dt_open.c | 10 ++-----
> libdtrace/dt_provider.c | 62 ++++++++++++++++++++++++-----------------
> libdtrace/dt_provider.h | 2 +-
> 4 files changed, 42 insertions(+), 35 deletions(-)
>
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 120f437719de..a500917f9c1c 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -315,8 +315,7 @@ struct dtrace_hdl {
> struct dt_probe *dt_error; /* ERROR probe */
>
> dt_list_t dt_provlist; /* linked list of dt_provider_t's */
> - struct dt_provider **dt_provs; /* hash table of dt_provider_t's */
> - uint_t dt_provbuckets; /* number of provider hash buckets */
> + dt_htab_t *dt_provs; /* hash table of dt_provider_t's */
> uint_t dt_nprovs; /* number of providers in hash and list */
> const struct dt_provider *dt_prov_pid; /* PID provider */
> dt_proc_hash_t *dt_procs; /* hash table of grabbed process handles */
> diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> index 1d16c55f650b..5df8fef14b9b 100644
> --- a/libdtrace/dt_open.c
> +++ b/libdtrace/dt_open.c
> @@ -736,8 +736,6 @@ dt_vopen(int version, int flags, int *errp,
> dtp->dt_poll_fd = -1;
> dtp->dt_modbuckets = _dtrace_strbuckets;
> dtp->dt_mods = calloc(dtp->dt_modbuckets, sizeof(dt_module_t *));
> - dtp->dt_provbuckets = _dtrace_strbuckets;
> - dtp->dt_provs = calloc(dtp->dt_provbuckets, sizeof(dt_provider_t *));
> dt_proc_hash_create(dtp);
> dtp->dt_proc_fd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
> dtp->dt_nextepid = 1;
> @@ -770,10 +768,9 @@ dt_vopen(int version, int flags, int *errp,
> if (dt_str2kver(dtp->dt_uts.release, &dtp->dt_kernver) < 0)
> return set_open_errno(dtp, errp, EDT_VERSINVAL);
>
> - if (dtp->dt_mods == NULL || dtp->dt_provs == NULL ||
> - dtp->dt_procs == NULL || dtp->dt_ld_path == NULL ||
> - dtp->dt_cpp_path == NULL || dtp->dt_cpp_argv == NULL ||
> - dtp->dt_sysslice == NULL)
> + if (dtp->dt_mods == NULL || dtp->dt_procs == NULL ||
> + dtp->dt_ld_path == NULL || dtp->dt_cpp_path == NULL ||
> + dtp->dt_cpp_argv == NULL || dtp->dt_sysslice == NULL)
> return set_open_errno(dtp, errp, EDT_NOMEM);
>
> for (i = 0; i < DTRACEOPT_MAX; i++)
> @@ -1286,7 +1283,6 @@ dtrace_close(dtrace_hdl_t *dtp)
> elf_end(dtp->dt_ctf_elf);
> free(dtp->dt_mods);
> free(dtp->dt_module_path);
> - free(dtp->dt_provs);
> free(dtp->dt_strtab);
> free(dtp);
>
> diff --git a/libdtrace/dt_provider.c b/libdtrace/dt_provider.c
> index 3d0836710503..88ae1bd4fa15 100644
> --- a/libdtrace/dt_provider.c
> +++ b/libdtrace/dt_provider.c
> @@ -22,13 +22,36 @@
> #include <dt_string.h>
> #include <dt_list.h>
>
> +static uint32_t
> +dt_provider_hval(const dt_provider_t *pvp)
> +{
> + return str2hval(pvp->desc.dtvd_name, 0);
> +}
> +
> +static int
> +dt_provider_cmp(const dt_provider_t *p,
> + const dt_provider_t *q)
> +{
> + return strcmp(p->desc.dtvd_name, q->desc.dtvd_name);
> +}
> +
> +DEFINE_HE_STD_LINK_FUNCS(dt_provider, dt_provider_t, he)
> +DEFINE_HTAB_STD_OPS(dt_provider)
> +
> static dt_provider_t *
> -dt_provider_insert(dtrace_hdl_t *dtp, dt_provider_t *pvp, uint_t h)
> +dt_provider_insert(dtrace_hdl_t *dtp, dt_provider_t *pvp)
> {
> - dt_list_append(&dtp->dt_provlist, pvp);
> + if (!dtp->dt_provs) {
> + dtp->dt_provs = dt_htab_create(dtp, &dt_provider_htab_ops);
> + if (dtp->dt_provs == NULL)
> + return NULL;
> + }
>
> - pvp->pv_next = dtp->dt_provs[h];
> - dtp->dt_provs[h] = pvp;
> + if (dt_htab_insert(dtp->dt_provs, pvp) < 0) {
> + free(pvp);
> + return NULL;
> + }
> + dt_list_append(&dtp->dt_provlist, pvp);
> dtp->dt_nprovs++;
>
> return pvp;
> @@ -37,15 +60,13 @@ dt_provider_insert(dtrace_hdl_t *dtp, dt_provider_t *pvp, uint_t h)
> dt_provider_t *
> dt_provider_lookup(dtrace_hdl_t *dtp, const char *name)
> {
> - uint_t h = str2hval(name, 0) % dtp->dt_provbuckets;
> - dt_provider_t *pvp;
> + dt_provider_t tmpl;
>
> - for (pvp = dtp->dt_provs[h]; pvp != NULL; pvp = pvp->pv_next) {
> - if (strcmp(pvp->desc.dtvd_name, name) == 0)
> - return pvp;
> - }
> + if ((strlen (name) + 1) > sizeof (tmpl.desc.dtvd_name))
> + return NULL;
>
> - return NULL;
> + strcpy (tmpl.desc.dtvd_name, name);
> + return dt_htab_lookup(dtp->dt_provs, &tmpl);
> }
>
> dt_provider_t *
> @@ -71,27 +92,15 @@ dt_provider_create(dtrace_hdl_t *dtp, const char *name,
>
> memcpy(&pvp->desc.dtvd_attr, pattr, sizeof(dtrace_pattr_t));
>
> - return dt_provider_insert(dtp, pvp,
> - str2hval(name, 0) % dtp->dt_provbuckets);
> + return dt_provider_insert(dtp, pvp);
> }
>
> void
> dt_provider_destroy(dtrace_hdl_t *dtp, dt_provider_t *pvp)
> {
> - dt_provider_t **pp;
> - uint_t h;
> -
> assert(pvp->pv_hdl == dtp);
>
> - h = str2hval(pvp->desc.dtvd_name, 0) % dtp->dt_provbuckets;
> - pp = &dtp->dt_provs[h];
> -
> - while (*pp != NULL && *pp != pvp)
> - pp = &(*pp)->pv_next;
> -
> - assert(*pp != NULL && *pp == pvp);
> - *pp = pvp->pv_next;
> -
> + dt_htab_delete(dtp->dt_provs, pvp);
> dt_list_delete(&dtp->dt_provlist, pvp);
> dtp->dt_nprovs--;
>
> @@ -101,6 +110,9 @@ dt_provider_destroy(dtrace_hdl_t *dtp, dt_provider_t *pvp)
> dt_node_link_free(&pvp->pv_nodes);
> dt_free(dtp, pvp->pv_xrefs);
> dt_free(dtp, pvp);
> +
> + if (dtp->dt_nprovs == 0)
> + dt_htab_destroy(dtp, dtp->dt_provs);
> }
>
> int
> diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
> index a24b70f9f9d3..d366b378a46b 100644
> --- a/libdtrace/dt_provider.h
> +++ b/libdtrace/dt_provider.h
> @@ -87,7 +87,7 @@ extern dt_provimpl_t dt_syscall;
>
> typedef struct dt_provider {
> dt_list_t pv_list; /* list forward/back pointers */
> - struct dt_provider *pv_next; /* pointer to next provider in hash */
> + struct dt_hentry he; /* htab links */
> dtrace_providerdesc_t desc; /* provider name and attributes */
> const dt_provimpl_t *impl; /* provider implementation */
> dt_idhash_t *pv_probes; /* probe defs (if user-declared) */
More information about the DTrace-devel
mailing list