Re: [PATCH v2 7/8] livepatch: Improve the search performance of module_kallsyms_on_each_symbol()

[Leizhen (ThunderTown)]

On 2022/9/20 20:08, Petr Mladek wrote:
> On Fri 2022-09-09 21:00:15, Zhen Lei wrote:
>> Currently we traverse all symbols of all modules to find the specified
>> function for the specified module. But in reality, we just need to find
>> the given module and then traverse all the symbols in it.
> 
> I agree that it might be noticeable speedup.
> 
>> In order to achieve this purpose, split the call to hook 'fn' into two
>> phases:
>> 1. Finds the given module. Pass pointer 'mod'. Hook 'fn' directly returns
>>    the comparison result of the module name without comparing the function
>>    name.
>> 2. Finds the given function in that module. Pass pointer 'mod = NULL'.
>>    Hook 'fn' skip the comparison of module name and directly compare
>>    function names.
>>
>> Phase1: mod1-->mod2..(subsequent modules do not need to be compared)
>>                 |
>> Phase2:          -->f1-->f2-->f3
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
>> ---
>>  kernel/livepatch/core.c  |  7 ++-----
>>  kernel/module/kallsyms.c | 13 ++++++++++++-
>>  2 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index 31b57ccf908017e..98e23137e4133bc 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -130,15 +130,12 @@ static int klp_find_callback(void *data, const char *name,
>>  {
>>  	struct klp_find_arg *args = data;
>>  
>> -	if ((mod && !args->objname) || (!mod && args->objname))
>> -		return 0;
>> +	if (mod)
>> +		return strcmp(args->objname, mod->name);
>>  
>>  	if (strcmp(args->name, name))
>>  		return 0;
>>  
>> -	if (args->objname && strcmp(args->objname, mod->name))
>> -		return 0;
>> -
>>  	args->addr = addr;
>>  	args->count++;
>>  
>> diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
>> index f5c5c9175333df7..b033613e6c7e3bb 100644
>> --- a/kernel/module/kallsyms.c
>> +++ b/kernel/module/kallsyms.c
>> @@ -510,6 +510,11 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
>>  		if (mod->state == MODULE_STATE_UNFORMED)
>>  			continue;
>>  
>> +		/* check mod->name first */
>> +		ret = fn(data, NULL, mod, 0);
>> +		if (ret)
>> +			continue;
> 
> Hmm, it somehow gets too complicated. The same fn() callback has to
> behave correctly in 3 different situations. I would suggest to
> simplify everything:
> 
> 
> 1. Pass the requested modname as a parameter to module_kallsyms_on_each_symbol()
> 
> /*
>  * Iterate over all symbols in the given @modname. For symbols from
>  * vmlinux use kallsyms_on_each_symbol() instead.
>  */
> int module_kallsyms_on_each_symbol(const char *modname,
> 				   int (*fn)(void *, const char *,
> 					     struct module *, unsigned long),
> 				   void *data)
> 
> and do here:
> 
> 		if (strcmp(modname, mod->name))
> 			continue;

Right, looks good. This makes the code logic much clearer. Thanks.

> 
> 
> 2. We do not even need to pass .objname in struct klp_find_arg
>    could simplify the callback:
> 

Yes

> static int klp_find_callback(void *data, const char *name,
> 			     struct module *mod, unsigned long addr)
> {
> 	struct klp_find_arg *args = data;
> 
> 	if (strcmp(args->name, name))
> 		return 0;
> 
> 	args->addr = addr;
> 	args->count++;
> 
> 	/*
> 	 * Finish the search when the symbol is found for the desired position
> 	 * or the position is not defined for a non-unique symbol.
> 	 */
> 	if ((args->pos && (args->count == args->pos)) ||
> 	    (!args->pos && (args->count > 1)))
> 		return 1;
> 
> 	return 0;
> }
> 
> 3. As a result the *mod parameter won't be used by any existing
>    fn() callback and could be removed. This should be done as
>    a separate patch. It touches also ftrace_lookup_symbols().

OK, I will do it tomorrow. The next version is v5.

> 
> Best Regards,
> Petr
> .
> 

-- 
Regards,
  Zhen Lei