[DTrace-devel] [PATCH v2 07/11] htab reduction: kernpath
Kris Van Hees
kris.van.hees at oracle.com
Thu Oct 21 22:39:59 PDT 2021
On Fri, Oct 22, 2021 at 12:15:51AM -0400, Eugene Loh wrote:
> 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.
THe htab implementation was created to support data elements that can be
put in multiple htabs, as we do for probes. For the probe example, each of
the htabs hashes on a different string in the probe desc, and actually there
is one htab that the probe gets added to that hashes across all strings in the
probe desc. So, there a a high degree of flexibility that needs to be
provided, and while it can be nice to provide some consistency in places
where it can be done, I am not sure that is really worth the effort.
> 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).
Why not simply del? Whether a delete function has other resources to free
or not shouldn't matter to the htab implementation. You call the op to
delete the element, and the implementation of the ops will do what is needed
for that particular type.
> 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);
>
> _______________________________________________
> 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