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

[Nick Alcock]

On 25 Oct 2021, Eugene Loh uttered the following:

> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> And...
>
> On 10/20/21 2:54 PM, Nick Alcock wrote:
>> The symtab is a major performance sink.  The symtabs in a running DTrace
>> vary from tiny (most modules) to huge (20,000+ entries in the vmlinux
>> symtab), but all of them are stuffed into fixed-size htabs of size 211,
>> leading to hash chains hundreds long.  Combine that with the exponential
>
> I already asked about this.  Why exponential?  You said it's O(nm). That 
> is not exponential.  Exponential would be O(2^n)... or so.

True! I'll delete the word :)

>> The only extra bit of fragility this adds is that destruction of the
>> centralized dt_kernsym_t makes all symbol name lookups fail, since we no
>> longer have any per-symbol hashtabs.  So we add a new dt_module_fini
>> that ensures that modules are destroyed right after the kernsym hashtab,
>> so there is no room for any name lookups to sneak in.
>
> I do not understand this.  Nor do I see a dt_module_fini().

... because I deleted it :) this para is residual from before everything
was using destructors.  What this is actually saying is that if you
destroy the dt_kernsyms hash but not the dt_mods hash, lookups will
fail: but since we never do that and doing that would be silly this is
now a pointless paragraph.  Deleted.

>> diff --git a/libdtrace/dt_symtab.c b/libdtrace/dt_symtab.c
>> @@ -29,14 +29,13 @@
>>   
>>   struct dt_symbol {
>>   	dt_list_t dts_list;		/* list forward/back pointers */
>
> Not a part of this patch, but so far as I can tell dts_list has been 
> orphaned since its introduction a decade ago in 9f987758bc9a  .

They're the list pointers for dtst_symlist.  This sort of hidden usage
based on position is horrible and dt_list is rife with it :(

>>   struct dt_symtab {
>>   	dt_list_t dtst_symlist;		/* symbol list */
>
> I do not understand why dtst_symlist is still included.  Can't it be 
> replaced by the iterator?

Not in this case: the kernsyms hashtab is dtp-wide and covers *all
symbols*, in whichever module. The dtst_symlist chains together symbosl
in one specific module. Since things like packing still happen on a
per-mdoule basis (so we can, in theory, change the list and e.g. respond
to module loads while dtrace is running), we need something that covers
individual modules.