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
