Message-ID: <58DA5C25.5020106@huawei.com>
Date: Tue, 28 Mar 2017 20:50:45 +0800
From: zhouchengming <zhouchengming1@huawei.com>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20120428 Thunderbird/12.0.1
MIME-Version: 1.0
To: Miroslav Benes <mbenes@suse.cz>
CC: <live-patching@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <jpoimboe@redhat.com>, <jeyu@redhat.com>, <jikos@kernel.org>,
        <pmladek@suse.com>, <huawei.libin@huawei.com>
Subject: Re: [PATCH] reduce the time of finding symbols for module
References: <1490666535-19192-1-git-send-email-zhouchengming1@huawei.com> <alpine.LSU.2.20.1703281055550.30237@pobox.suse.cz> <58DA41EE.90607@huawei.com> <alpine.LSU.2.20.1703281308060.30237@pobox.suse.cz>
In-Reply-To: <alpine.LSU.2.20.1703281308060.30237@pobox.suse.cz>
Content-Type: text/plain; charset="ISO-8859-1"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3714
Lines: 100

On 2017/3/28 19:16, Miroslav Benes wrote:
> 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.
>

Yes, vmlinux kallsyms table is too big, but modules loaded are always few.

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

Ok, if there are no oppositions, I will send a patch-v2 with improved changelog.
This is really a temporary solution, and others can go on to fix the kallsyms
if needed later.

Thanks.

> Thanks,
> Miroslav
>
> .
>