[DTrace-devel] [PATCH 5/7] htab reduction: modules

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

That said, I still wonder about maintaining the "unnecessary" dt_modlist 
and dt_nmods.  Admittedly, the situation is different here since you 
care about a certain order of modules.  So, maybe dt_modlist is 
necessary after all.  Nevertheless, if you do introduce an htab 
destroyer or iterator, that could be used in dt_modlist's place where 
appropriate?  Dunno.  Up to you. Meanwhile, the case for preserving 
dt_nmods seems weaker.

On 10/5/21 9:38 AM, Nick Alcock wrote:
> This one is nice and simple, just the same as all the others.
This line in the commit message is not needed.
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> @@ -113,7 +113,8 @@ typedef struct dt_module {
>   	char dm_name[DTRACE_MODNAMELEN]; /* string name of module */
>   	char dm_file[PATH_MAX]; /* file path of module */
>   	uint_t dm_flags;	/* module flags (see below) */
> -	struct dt_module *dm_next; /* pointer to next module in hash chain */
> +	struct dt_hentry dm_he;	/* htab links */
> +
>   
>   	dtrace_addr_range_t *dm_text_addrs; /* text addresses, sorted */
>   	size_t dm_text_addrs_size;	 /* number of entries */
Extraneous blank line.
> diff --git a/libdtrace/dt_module.c b/libdtrace/dt_module.c
> @@ -40,6 +40,22 @@ dt_module_shuffle_to_start(dtrace_hdl_t *dtp, const char *name);
> +static int
> +dt_module_cmp(const dt_module_t *p,
> +	      const dt_module_t *q)
> +{
> +	return strcmp(p->dm_name, q->dm_name);
> +}
No need to split such a short line?
> @@ -96,21 +112,25 @@ dt_module_symgelf64(const Elf64_Sym *src, GElf_Sym *dst)
>   dt_module_t *
>   dt_module_create(dtrace_hdl_t *dtp, const char *name)
>   {
> -	uint_t h = str2hval(name, 0) % dtp->dt_modbuckets;
>   	dt_module_t *dmp;
>   
> -	for (dmp = dtp->dt_mods[h]; dmp != NULL; dmp = dmp->dm_next)
> -		if (strcmp(dmp->dm_name, name) == 0)
> -			return dmp;
> +	if (!dtp->dt_mods &&
> +	    ((dtp->dt_mods = dt_htab_create(dtp, &dt_module_htab_ops)) == NULL))
> +		return NULL;

Okay, but stylistically it'd be nice to be relentlessly consistent.  In 
the other patches, you wrote this more like
     if (!dtp->dt_mods) {
         dtp->dt_mods = ...;
         if (!dtp->dt_mods) ...
     }
So, perhaps mimic that style here?  Or (since this new style is more 
compact) rewrite this stuff in the other patches to look more like this 
more compact style?  Or, just forget about this picky feedback altogether!

> @@ -124,8 +144,13 @@ dt_module_create(dtrace_hdl_t *dtp, const char *name)
>   dt_module_t *
>   dt_module_lookup_by_name(dtrace_hdl_t *dtp, const char *name)
>   {
> -	uint_t h = str2hval(name, 0) % dtp->dt_modbuckets;
> -	dt_module_t *dmp;
> +	dt_module_t skel;
Regarding relentless stylistic consistency, this is "tmpl" in the other 
patches.  I really think you should also use "tmpl" here as well instead 
of "skel".
> @@ -133,11 +158,8 @@ dt_module_lookup_by_name(dtrace_hdl_t *dtp, const char *name)
> +	strcpy(skel.dm_name, name);
> +	return dt_htab_lookup(dtp->dt_mods, &skel);
>   }