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

[Petr Mladek <pmladek@suse.com>]

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@huawei.com>
> ---
>  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;


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

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

Best Regards,
Petr