Re: [PATCH] reduce the time of finding symbols for module

[Miroslav Benes]

On Tue, 28 Mar 2017, zhouchengming wrote:

> On 2017/3/28 17:00, Miroslav Benes wrote:
> > 
> > Hi,
> > 
> > On Tue, 28 Mar 2017, Zhou Chengming wrote:
> > 
> > > It's reported that the time of insmoding a klp.ko for one of our
> > > out-tree modules is too long.
> > > 
> > > ~ time sudo insmod klp.ko
> > > real	0m23.799s
> > > user	0m0.036s
> > > sys	0m21.256s
> > 
> > Is this stable through several (>=10) runs? 23 seconds are really
> > suspicious. Yes, there is a linear search through all the kallsyms in
> > kallsyms_on_each_symbol(), but there are something like 70k symbols on my
> > machine (that is, way less than 1M). 23 seconds are somewhat unexpected.
> > 
> 
> Yes, it's stable through several runs.
> 
> I think the big reason is that our out-tree module used a lot of static local
> variables. We can see '.rela.kpatch.dynrelas' contains many entries, so it
> will
> waste a lot of time if we use kallsyms_on_each_symbol() to find these symbols
> of module.

Ok, it means that you have a lot of relocation records which reference 
your out-of-tree module. Then for each such entry klp_resolve_symbol() 
is called and then klp_find_object_symbol() to actually resolve it. So if 
you have 20k entries, you walk through vmlinux kallsyms table 20k times. 
It is unneeded and that is why your fix works.

But if there were 20k modules loaded, the problem would still be there.

I think it would be really nice to fix kallsyms :). Replace ordinary array 
and the linear search with a hash table.
 
> Relocation section '.rela.kpatch.funcs' at offset 0x382e0 contains 3 entries:
>   Offset          Info           Type           Sym. Value    Sym. Name +
> Addend
> 000000000000  003300000101 R_AARCH64_ABS64   0000000000000000 value_show + 0
> 000000000020  000b00000101 R_AARCH64_ABS64   0000000000000000 .kpatch.strings
> + 8
> 000000000028  000b00000101 R_AARCH64_ABS64   0000000000000000 .kpatch.strings
> + 0

Hm, we do not have aarch64 support in upstream (yet). There is even no 
dynamic ftrace with regs yet (if I am not mistaken).

> Relocation section '.rela.kpatch.dynrelas' at offset 0x38328 contains 2562
> entries:
>   Offset          Info           Type           Sym. Value    Sym. Name +
> Addend
> 000000000000  003300000101 R_AARCH64_ABS64   0000000000000000 value_show + 14
> 000000000018  000b00000101 R_AARCH64_ABS64   0000000000000000 .kpatch.strings
> + 13
> 000000000020  000b00000101 R_AARCH64_ABS64   0000000000000000 .kpatch.strings
> + 0
> 000000000040  003300000101 R_AARCH64_ABS64   0000000000000000 value_show + 20
> 000000000058  000b00000101 R_AARCH64_ABS64   0000000000000000 .kpatch.strings
> + 13
> 000000000060  000b00000101 R_AARCH64_ABS64   0000000000000000 .kpatch.strings
> + 0
> 
> > If it is a problem, can we fix kallsyms_on_each_symbol() and replace the
> > linear search with something better? All users would benefit...
> > 
> 
> Yes, it's better if we can improve the linear search, but I can't think of
> that...

I don't understand. Fixing kallsyms is of course much more work but 
everyone would benefit from that.

If there is an agreement, we could accept your solution as temporary. In 
such case, please prefix the subject with 'livepatch: ' and use capital 
letter in 'Reduce'. Please also improve the changelog and describe where 
the problem really is.

Thanks,
Miroslav