| Message ID | 20220909130016.727-8-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kallsyms: Optimizes the performance of lookup symbols | expand |
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
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@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; 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 > . >
On Tue 2022-09-20 22:01:44, Leizhen (ThunderTown) wrote: > On 2022/9/20 20:08, Petr Mladek wrote: > > On Fri 2022-09-09 21:00:15, Zhen Lei wrote: > > 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. Please, wait a bit. I have missed that there was already v4 that solved some my concerns. I am going to look at it. Best Regards, Petr
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; + /* Use rcu_dereference_sched() to remain compliant with the sparse tool */ preempt_disable(); kallsyms = rcu_dereference_sched(mod->kallsyms); @@ -522,10 +527,16 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, continue; ret = fn(data, kallsyms_symbol_name(kallsyms, i), - mod, kallsyms_symbol_value(sym)); + NULL, kallsyms_symbol_value(sym)); if (ret != 0) goto out; } + + /* + * The given module is found, the subsequent modules do not + * need to be compared. + */ + break; } out: mutex_unlock(&module_mutex);
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. 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(-)