[DTrace-devel] [PATCH v2 09/11] htab reduction: providers

[Eugene Loh]

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

On 10/20/21 2:54 PM, 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.

Personal taste:  I'd prefer a full stop in place of that colon.

Also, the "list of providers" was separate from the hash table.

How about:
         Providers were stored in fixed-size hash tables and companion
         lists.  The fixed-size tables generally were grossly oversized;
         with USDT or pid, they were terribly undersized.

         Eliminate the lists and replace the hash tables with dt_htab.

But, your call.

> We can move to using destructors (allowing deletion of htab elements
> without needing a specialized _destroy function, and straightforward
> freeing of the dt_provs htab on dtrace_close), but this is a little
> troublesome because the dt_provlist is separate from dt_provs,
> inaccessible from dt_provs element destructors (which do not have access
> to a dtp), yet its members are added/removed from the dtp as we go.
>
> We can fix this by dropping the dt_provlist and replacing it with
> dt_htab_next-based iteration over dt_provs. The iteration order is
> different, but that's all that changes.
>
> (This is... debatable, and I can see why people might prefer to keep the
> old design and just keep a dtp backpointer in every provider element.
> I'm happy with that approach too.)

I'm having trouble reading those three paragraphs.  What I do know is 
that some stuff that was allocated with dt_zalloc(dtp, ...) and freed 
with dt_free(dtp, ...) is now sometimes (but not always) freed with 
free(...).  I guess that's okay -- dt_free() is really just a free() -- 
but that seems like a sufficiently big "architectural change" that... I 
dunno.  Anyhow, I can see why you flag the issue, even if I don't 
understand this explanation of it.

> diff --git a/libdtrace/dt_dof.c b/libdtrace/dt_dof.c
> @@ -560,6 +560,7 @@ dtrace_dof_create(dtrace_hdl_t *dtp, dtrace_prog_t *pgp, uint_t flags)
>   	uint_t maxfmt = 0;
>   #endif
>   
> +	dt_htab_next_t *it = NULL;
>   	dt_provider_t *pvp;
>   	dt_xlator_t *dxp;
>   	dof_actdesc_t *dofa;

How about moving that declaration down into the clause where it is used?

> @@ -746,8 +747,7 @@ dtrace_dof_create(dtrace_hdl_t *dtp, dtrace_prog_t *pgp, uint_t flags)
>   	 * to the providers and the probes and arguments that they define.
>   	 */
>   	if (flags & DTRACE_D_PROBES) {
> -		for (pvp = dt_list_next(&dtp->dt_provlist);
> -		    pvp != NULL; pvp = dt_list_next(pvp))
> +		while ((pvp = dt_htab_next(dtp->dt_provs, &it)) != NULL)
>   			dof_add_provider(ddo, pvp);
>   	}

Into this clause.

> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> @@ -324,9 +324,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 */

The comment "and list" for dt_nprovs no longer makes sense since there 
is no more list.  Actually, dt_nprovs is not even used for anything.  
IMHO, it can be removed entirely.

> diff --git a/libdtrace/dt_pcb.c b/libdtrace/dt_pcb.c
> @@ -127,8 +128,7 @@ dt_pcb_pop(dtrace_hdl_t *dtp, int err)
> -		for (pvp = dt_list_next(&dtp->dt_provlist); pvp; pvp = nvp) {
> -			nvp = dt_list_next(pvp);
> +		while ((pvp = dt_htab_next(dtp->dt_provs, &it)) != NULL) {
>   			if (pvp->pv_gen == dtp->dt_gen) {
>   				dtrace_probedesc_t	pdp;
>   
> @@ -141,7 +141,7 @@ dt_pcb_pop(dtrace_hdl_t *dtp, int err)
>   				dt_probe_iter(dtp, &pdp, dt_pcb_destroy_probe,
>   					      NULL, NULL);
>   
> -				dt_provider_destroy(dtp, pvp);
> +				dt_htab_next_delete(it);
>   			}
>   		}

This code is okay, but it's an interesting example of some things I 
mentioned.  E.g., the old code would destroy pvp, which was okay since 
it already picked up nvp.  Using a similar approach, one would not need 
a special dt_htab_next_delete() function.  Alternatively, have the 
iterator stop at the entry *after* the one that is returned, and then 
you could delete entries as you iterated through the list.