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

[Nick Alcock]

On 22 Oct 2021, Eugene Loh said:

> 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.

Agreed. I seem to have colons on the brain right now.

> 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.

Yours is better.

>> 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 

Rephrased a bit:

    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, and since
    dt_provs element destructors don't have access to a dtp, the destructors
    also can't get at the dt_provlist to unchain dt_provs elements that are
    being deleted.

> 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.

We have no choice: the place that does the free has no way to get access
to a dtp unless we explicitly start dragging around dtp back-pointers
and wasting space just so we can pass it to a function that then does
nothing with it. Since the wrappers are useless and used only
inconsistently I wouldn't call using them less an architectural change :)

I hate the dt_ allocate/free wrappers with a passion. I'd really like to
coccinelle up the source tree and just delete the lot, or at least
delete their unused-but-if-you-pass-NULL-it-kills-you dtp argument.

>> 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?

Good idea.

>> 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.

Yes indeed! It's gone.

>> @@ -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.

With the htab iterator now keeping track of the element it'll return on
the next call, this is now:

		dt_htab_delete(dtp->dt_provs, pvp);

... which is probably at least a bit clearer.

-- 
NULL && (void)