[DTrace-devel] [PATCH v2 07/11] htab reduction: kernpath
Eugene Loh
eugene.loh at oracle.com
Thu Oct 21 21:15:51 PDT 2021
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
On 10/20/21 2:54 PM, Nick Alcock wrote:
> We start out reducing hand-rolled htabs by dropping a simple one,
> the kernpath hash.
How about:
Replace the special-purpose kernpath hash table with the
more general dt_htab.
Incidentally, it would be tempting to enforce more consistency among
various data types that will use dt_htab so that any data type that
needs hval() and cmp() functions based on a string could use the same
macros (which would find the string pointers at predictable offsets),
but I suppose that's too much to ask for. Just a thought, but probably
not a worthwhile one.
There is, however, some other consistency that might be reasonable in
these various patches. E.g., in dt_htab.h, del always comes before
next. So, how about using the same order in other files as well -- that
is, wherever "dt_htab_ops_t *_htab_ops" are defined.
But come to think of it, when you define delete functions that free
resources, you append customized suffixes: del_buf, del_path, del_mod,
and del_prov. Why? How about making them all del_res (or something).
Then, the dt_htab_ops_t definitions can all use a common macro:
DEFINE_HTAB_STD_OPS_RES(ID) (or something like that).
I know it isn't worth spending one's entire career on these hash tables,
but maybe there are some opportunities here to streamline the use of
hash tables ruthlessly for simpler usage and a cleaner code base. Your
call.
> This has very little effect on performance, because even though the htab
> is terribly poorly sized, it is sized in the wrong direction: the
> kernpath hash only contains out-of-tree modules, and there won't be
> hundreds of those. The main improvement here is in clarity.
>
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
> libdtrace/dt_impl.h | 7 +--
> libdtrace/dt_kernel_module.c | 109 +++++++++++++++++++----------------
> libdtrace/dt_kernel_module.h | 3 +-
> libdtrace/dt_open.c | 15 ++---
> 4 files changed, 66 insertions(+), 68 deletions(-)
>
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 7c281e22c98e..36a445270b11 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -159,7 +159,7 @@ typedef struct dt_module {
> */
> typedef struct dt_kern_path {
> dt_list_t dkp_list; /* list forward/back pointers */
> - struct dt_kern_path *dkp_next; /* hash chain next pointer */
> + struct dt_hentry dkp_he; /* htab links */
> char *dkp_name; /* name of this kernel module */
> char *dkp_path; /* full name including path */
> } dt_kern_path_t;
> @@ -300,10 +300,7 @@ struct dtrace_hdl {
> ctf_archive_t *dt_ctfa; /* ctf archive for the entire kernel tree */
> ctf_file_t *dt_shared_ctf; /* Handle to the shared CTF */
> char *dt_ctfa_path; /* path to vmlinux.ctfa */
> - dt_list_t dt_kernpathlist; /* linked list of dt_kern_path_t's */
> - dt_kern_path_t **dt_kernpaths; /* hash table of dt_kern_path_t's */
> - uint_t dt_kernpathbuckets; /* number of kernel module path hash buckets */
> - uint_t dt_nkernpaths; /* number of kernel module paths in hash and list */
> + dt_htab_t *dt_kernpaths; /* hash table of dt_kern_path_t's */
> dt_module_t *dt_exec; /* pointer to executable module */
> dt_module_t *dt_cdefs; /* pointer to C dynamic type module */
> dt_module_t *dt_ddefs; /* pointer to D dynamic type module */
> diff --git a/libdtrace/dt_kernel_module.c b/libdtrace/dt_kernel_module.c
> index 47a4cc04e0bf..1d147405af9d 100644
> --- a/libdtrace/dt_kernel_module.c
> +++ b/libdtrace/dt_kernel_module.c
> @@ -29,66 +29,78 @@
>
> static pthread_mutex_t kern_path_update_lock = PTHREAD_MUTEX_INITIALIZER;
>
> +static uint32_t
> +kernpath_hval(const dt_kern_path_t *dkp)
> +{
> + return str2hval(dkp->dkp_name, 0);
> +}
> +
> +static int
> +kernpath_cmp(const dt_kern_path_t *p,
> + const dt_kern_path_t *q)
> +{
> + return strcmp(p->dkp_name, q->dkp_name);
> +}
> +
> +DEFINE_HE_STD_LINK_FUNCS(kernpath, dt_kern_path_t, dkp_he)
> +
> +static void *
> +kernpath_del_path(dt_kern_path_t *head, dt_kern_path_t *dkpp)
> +{
> + head = kernpath_del(head, dkpp);
> + free(dkpp->dkp_name);
> + free(dkpp->dkp_path);
> + free(dkpp);
> + return head;
> +}
> +
> +static dt_htab_ops_t kernpath_htab_ops = {
> + .hval = (htab_hval_fn)kernpath_hval,
> + .cmp = (htab_cmp_fn)kernpath_cmp,
> + .add = (htab_add_fn)kernpath_add,
> + .del = (htab_del_fn)kernpath_del_path
> +};
> +
> /*
> * On successful return, name and path become owned by dt_kern_path_create()
> - * (and may be freed immediately).
> - *
> - * Note: this code is used by ctf_module_dump as well.
> + * (and may be freed immediately). The returned entry is owned by the
> + * dt_kernpaths hash into which it is interned.
> */
>
> dt_kern_path_t *
> dt_kern_path_create(dtrace_hdl_t *dtp, char *name, char *path)
> {
> - uint_t h = str2hval(name, 0) % dtp->dt_kernpathbuckets;
> dt_kern_path_t *dkpp;
> + dt_kern_path_t tmpl;
>
> - for (dkpp = dtp->dt_kernpaths[h]; dkpp != NULL; dkpp = dkpp->dkp_next) {
> - if (strcmp(dkpp->dkp_name, name) == 0) {
> - free(name);
> - free(path);
> - return dkpp;
> - }
> + if (!dtp->dt_kernpaths) {
> + dtp->dt_kernpaths = dt_htab_create(dtp, &kernpath_htab_ops);
> +
> + if (!dtp->dt_kernpaths)
> + return NULL; /* caller must handle allocation failure */
> + }
> +
> + tmpl.dkp_name = name;
> + if ((dkpp = dt_htab_lookup(dtp->dt_kernpaths, &tmpl)) != NULL) {
> + free(name);
> + free(path);
> + return dkpp;
> }
>
> if ((dkpp = malloc(sizeof(dt_kern_path_t))) == NULL)
> - return NULL; /* caller must handle allocation failure */
> + return NULL;
>
> dt_dprintf("Adding %s -> %s\n", name, path);
>
> memset(dkpp, 0, sizeof(dt_kern_path_t));
> dkpp->dkp_name = name;
> dkpp->dkp_path = path; /* strdup()ped by our caller */
> - dt_list_append(&dtp->dt_kernpathlist, dkpp);
> - dkpp->dkp_next = dtp->dt_kernpaths[h];
> - dtp->dt_kernpaths[h] = dkpp;
> - dtp->dt_nkernpaths++;
> -
> - return dkpp;
> -}
> -
> -void
> -dt_kern_path_destroy(dtrace_hdl_t *dtp, dt_kern_path_t *dkpp)
> -{
> - uint_t h = str2hval(dkpp->dkp_name, 0) % dtp->dt_kernpathbuckets;
> - dt_kern_path_t *scan_dkpp;
> - dt_kern_path_t *prev_dkpp = NULL;
> -
> - dt_list_delete(&dtp->dt_kernpathlist, dkpp);
> - assert(dtp->dt_nkernpaths != 0);
> - dtp->dt_nkernpaths--;
> -
> - for (scan_dkpp = dtp->dt_kernpaths[h]; (scan_dkpp != NULL) &&
> - (scan_dkpp != dkpp); scan_dkpp = scan_dkpp->dkp_next) {
> - prev_dkpp = scan_dkpp;
> + if (dt_htab_insert(dtp->dt_kernpaths, dkpp) < 0) {
> + free(dkpp);
> + return NULL;
> }
> - if (prev_dkpp == NULL)
> - dtp->dt_kernpaths[h] = dkpp->dkp_next;
> - else
> - prev_dkpp->dkp_next = dkpp->dkp_next;
>
> - free(dkpp->dkp_name);
> - free(dkpp->dkp_path);
> - free(dkpp);
> + return dkpp;
> }
>
> /*
> @@ -185,10 +197,9 @@ dt_kern_path_update(dtrace_hdl_t *dtp)
> dt_kern_path_t *
> dt_kern_path_lookup_by_name(dtrace_hdl_t *dtp, const char *name)
> {
> - uint_t h = str2hval(name, 0) % dtp->dt_kernpathbuckets;
> - dt_kern_path_t *dkpp;
> + dt_kern_path_t tmpl;
>
> - if (dtp->dt_nkernpaths == 0) {
> + if (!dtp->dt_kernpaths) {
> int dterrno;
>
> pthread_mutex_lock(&kern_path_update_lock);
> @@ -201,14 +212,10 @@ dt_kern_path_lookup_by_name(dtrace_hdl_t *dtp, const char *name)
> return NULL;
> }
>
> - dt_dprintf("Initialized %i kernel module paths\n",
> - dtp->dt_nkernpaths);
> - }
> -
> - for (dkpp = dtp->dt_kernpaths[h]; dkpp != NULL; dkpp = dkpp->dkp_next) {
> - if (strcmp(dkpp->dkp_name, name) == 0)
> - return dkpp;
> + dt_dprintf("Initialized %zi kernel module paths\n",
> + dt_htab_entries(dtp->dt_kernpaths));
> }
>
> - return NULL;
> + tmpl.dkp_name = (char *) name;
> + return dt_htab_lookup(dtp->dt_kernpaths, &tmpl);
> }
> diff --git a/libdtrace/dt_kernel_module.h b/libdtrace/dt_kernel_module.h
> index 3397106a708a..902457d722fc 100644
> --- a/libdtrace/dt_kernel_module.h
> +++ b/libdtrace/dt_kernel_module.h
> @@ -1,6 +1,6 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2012, 2014, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2012, 2021, 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.
> */
> @@ -15,6 +15,5 @@ extern dt_kern_path_t *dt_kern_path_create(dtrace_hdl_t *dtp, char *name,
> extern int dt_kern_path_update(dtrace_hdl_t *dtp);
> extern dt_kern_path_t *dt_kern_path_lookup_by_name(dtrace_hdl_t *dtp,
> const char *name);
> -extern void dt_kern_path_destroy(dtrace_hdl_t *dtp, dt_kern_path_t *dkpp);
>
> #endif /* _DT_KERNEL_MODULE_H */
> diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> index 25b3e5b8cf70..1681940f1f74 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_kernpathbuckets = _dtrace_strbuckets;
> - dtp->dt_kernpaths = calloc(dtp->dt_kernpathbuckets, sizeof(dt_kern_path_t *));
> dtp->dt_provbuckets = _dtrace_strbuckets;
> dtp->dt_provs = calloc(dtp->dt_provbuckets, sizeof(dt_provider_t *));
> dt_proc_hash_create(dtp);
> @@ -772,10 +770,10 @@ 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_kernpaths == 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_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)
> return set_open_errno(dtp, errp, EDT_NOMEM);
>
> for (i = 0; i < DTRACEOPT_MAX; i++)
> @@ -1185,7 +1183,6 @@ dtrace_close(dtrace_hdl_t *dtp)
> {
> dt_ident_t *idp, *ndp;
> dt_module_t *dmp;
> - dt_kern_path_t *dkpp;
> dt_provider_t *pvp;
> dtrace_prog_t *pgp;
> dt_xlator_t *dxp;
> @@ -1233,8 +1230,7 @@ dtrace_close(dtrace_hdl_t *dtp)
> while ((dmp = dt_list_next(&dtp->dt_modlist)) != NULL)
> dt_module_destroy(dtp, dmp);
>
> - while ((dkpp = dt_list_next(&dtp->dt_kernpathlist)) != NULL)
> - dt_kern_path_destroy(dtp, dkpp);
> + dt_htab_destroy(dtp, dtp->dt_kernpaths);
>
> if (dtp->dt_shared_ctf != NULL)
> ctf_close(dtp->dt_shared_ctf);
> @@ -1286,7 +1282,6 @@ dtrace_close(dtrace_hdl_t *dtp)
>
> free(dtp->dt_mods);
> free(dtp->dt_module_path);
> - free(dtp->dt_kernpaths);
> free(dtp->dt_provs);
> free(dtp->dt_strtab);
> free(dtp);
More information about the DTrace-devel
mailing list