| Message ID | 20221102084921.1615-8-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kallsyms: Optimizes the performance of lookup symbols | expand |
On Wed, Nov 02, 2022 at 04:49:19PM +0800, 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. hi, sorry for delayed answer, I did not notice this until Stephen's email about merge issue with recent bpf change [1] > > Let's add a new parameter 'const char *modname' to function > module_kallsyms_on_each_symbol(), then we can compare the module names we have use case for iterating all modules and their symbols when we want to resolve passed addresses for tracing we don't have 'modname' that we could pass, we need to iterate all modules so perhaps this could be made optional like with passing NULL for modname? > directly in this function and call hook 'fn' after matching. And the > parameter 'struct module *' in the hook 'fn' can also be deleted. we need 'struct module *' argument in the callback as well because we are taking the module reference if we trace function in it, so it wont get unloaded please let me know if I should do the change or can help in any way thanks, jirka [1] https://lore.kernel.org/lkml/20221114111350.38e44eec@canb.auug.org.au/ > > Phase1: mod1-->mod2..(subsequent modules do not need to be compared) > | > Phase2: -->f1-->f2-->f3 > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > include/linux/module.h | 4 ++-- > kernel/livepatch/core.c | 13 ++----------- > kernel/module/kallsyms.c | 15 ++++++++++++--- > 3 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/include/linux/module.h b/include/linux/module.h > index ec61fb53979a92a..0a3b44ff885a48c 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -879,8 +879,8 @@ static inline bool module_sig_ok(struct module *module) > } > #endif /* CONFIG_MODULE_SIG */ > > -int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, > - struct module *, unsigned long), > +int module_kallsyms_on_each_symbol(const char *modname, > + int (*fn)(void *, const char *, unsigned long), > void *data); > > #endif /* _LINUX_MODULE_H */ > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index 50bfc3481a4ee38..d4fe2d1b0e562bc 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -118,27 +118,19 @@ static struct klp_object *klp_find_object(struct klp_patch *patch, > } > > struct klp_find_arg { > - const char *objname; > const char *name; > unsigned long addr; > unsigned long count; > unsigned long pos; > }; > > -static int klp_find_callback(void *data, const char *name, > - struct module *mod, unsigned long addr) > +static int klp_find_callback(void *data, const char *name, unsigned long addr) > { > struct klp_find_arg *args = data; > > - if ((mod && !args->objname) || (!mod && args->objname)) > - return 0; > - > if (strcmp(args->name, name)) > return 0; > > - if (args->objname && strcmp(args->objname, mod->name)) > - return 0; > - > args->addr = addr; > args->count++; > > @@ -175,7 +167,6 @@ static int klp_find_object_symbol(const char *objname, const char *name, > unsigned long sympos, unsigned long *addr) > { > struct klp_find_arg args = { > - .objname = objname, > .name = name, > .addr = 0, > .count = 0, > @@ -183,7 +174,7 @@ static int klp_find_object_symbol(const char *objname, const char *name, > }; > > if (objname) > - module_kallsyms_on_each_symbol(klp_find_callback, &args); > + module_kallsyms_on_each_symbol(objname, klp_find_callback, &args); > else > kallsyms_on_each_match_symbol(klp_match_callback, name, &args); > > diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c > index f5c5c9175333df7..329cef573675d49 100644 > --- a/kernel/module/kallsyms.c > +++ b/kernel/module/kallsyms.c > @@ -495,8 +495,8 @@ unsigned long module_kallsyms_lookup_name(const char *name) > } > > #ifdef CONFIG_LIVEPATCH > -int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, > - struct module *, unsigned long), > +int module_kallsyms_on_each_symbol(const char *modname, > + int (*fn)(void *, const char *, unsigned long), > void *data) > { > struct module *mod; > @@ -510,6 +510,9 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, > if (mod->state == MODULE_STATE_UNFORMED) > continue; > > + if (strcmp(modname, mod->name)) > + continue; > + > /* Use rcu_dereference_sched() to remain compliant with the sparse tool */ > preempt_disable(); > kallsyms = rcu_dereference_sched(mod->kallsyms); > @@ -522,10 +525,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)); > + 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); > -- > 2.25.1 >
On 2022/11/14 15:47, Jiri Olsa wrote: > On Wed, Nov 02, 2022 at 04:49:19PM +0800, 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. > > hi, > sorry for delayed answer, I did not notice this until Stephen's email > about merge issue with recent bpf change [1] > >> >> Let's add a new parameter 'const char *modname' to function >> module_kallsyms_on_each_symbol(), then we can compare the module names > > we have use case for iterating all modules and their symbols when we > want to resolve passed addresses for tracing > > we don't have 'modname' that we could pass, we need to iterate all modules > > so perhaps this could be made optional like with passing NULL for modname? The deletion of modname was suggested by Petr Mladek. The reason is that everyone passes modname as NULL, there was no actual demand at the time. https://lkml.org/lkml/2022/9/20/682 > >> directly in this function and call hook 'fn' after matching. And the >> parameter 'struct module *' in the hook 'fn' can also be deleted. > > we need 'struct module *' argument in the callback as well because we are > taking the module reference if we trace function in it, so it wont get > unloaded > > please let me know if I should do the change or can help in any way It seems that we should take the module reference before invoking callback and put it after it is called, without passing modname. > > thanks, > jirka > > [1] https://lore.kernel.org/lkml/20221114111350.38e44eec@canb.auug.org.au/ > >> >> Phase1: mod1-->mod2..(subsequent modules do not need to be compared) >> | >> Phase2: -->f1-->f2-->f3 >> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> include/linux/module.h | 4 ++-- >> kernel/livepatch/core.c | 13 ++----------- >> kernel/module/kallsyms.c | 15 ++++++++++++--- >> 3 files changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/include/linux/module.h b/include/linux/module.h >> index ec61fb53979a92a..0a3b44ff885a48c 100644 >> --- a/include/linux/module.h >> +++ b/include/linux/module.h >> @@ -879,8 +879,8 @@ static inline bool module_sig_ok(struct module *module) >> } >> #endif /* CONFIG_MODULE_SIG */ >> >> -int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, >> - struct module *, unsigned long), >> +int module_kallsyms_on_each_symbol(const char *modname, >> + int (*fn)(void *, const char *, unsigned long), >> void *data); >> >> #endif /* _LINUX_MODULE_H */ >> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c >> index 50bfc3481a4ee38..d4fe2d1b0e562bc 100644 >> --- a/kernel/livepatch/core.c >> +++ b/kernel/livepatch/core.c >> @@ -118,27 +118,19 @@ static struct klp_object *klp_find_object(struct klp_patch *patch, >> } >> >> struct klp_find_arg { >> - const char *objname; >> const char *name; >> unsigned long addr; >> unsigned long count; >> unsigned long pos; >> }; >> >> -static int klp_find_callback(void *data, const char *name, >> - struct module *mod, unsigned long addr) >> +static int klp_find_callback(void *data, const char *name, unsigned long addr) >> { >> struct klp_find_arg *args = data; >> >> - if ((mod && !args->objname) || (!mod && args->objname)) >> - return 0; >> - >> if (strcmp(args->name, name)) >> return 0; >> >> - if (args->objname && strcmp(args->objname, mod->name)) >> - return 0; >> - >> args->addr = addr; >> args->count++; >> >> @@ -175,7 +167,6 @@ static int klp_find_object_symbol(const char *objname, const char *name, >> unsigned long sympos, unsigned long *addr) >> { >> struct klp_find_arg args = { >> - .objname = objname, >> .name = name, >> .addr = 0, >> .count = 0, >> @@ -183,7 +174,7 @@ static int klp_find_object_symbol(const char *objname, const char *name, >> }; >> >> if (objname) >> - module_kallsyms_on_each_symbol(klp_find_callback, &args); >> + module_kallsyms_on_each_symbol(objname, klp_find_callback, &args); >> else >> kallsyms_on_each_match_symbol(klp_match_callback, name, &args); >> >> diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c >> index f5c5c9175333df7..329cef573675d49 100644 >> --- a/kernel/module/kallsyms.c >> +++ b/kernel/module/kallsyms.c >> @@ -495,8 +495,8 @@ unsigned long module_kallsyms_lookup_name(const char *name) >> } >> >> #ifdef CONFIG_LIVEPATCH >> -int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, >> - struct module *, unsigned long), >> +int module_kallsyms_on_each_symbol(const char *modname, >> + int (*fn)(void *, const char *, unsigned long), >> void *data) >> { >> struct module *mod; >> @@ -510,6 +510,9 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, >> if (mod->state == MODULE_STATE_UNFORMED) >> continue; >> >> + if (strcmp(modname, mod->name)) >> + continue; >> + >> /* Use rcu_dereference_sched() to remain compliant with the sparse tool */ >> preempt_disable(); >> kallsyms = rcu_dereference_sched(mod->kallsyms); >> @@ -522,10 +525,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)); >> + 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); >> -- >> 2.25.1 >> > . >
On Mon, Nov 14, 2022 at 04:50:25PM +0800, Leizhen (ThunderTown) wrote: > > > On 2022/11/14 15:47, Jiri Olsa wrote: > > On Wed, Nov 02, 2022 at 04:49:19PM +0800, 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. > > > > hi, > > sorry for delayed answer, I did not notice this until Stephen's email > > about merge issue with recent bpf change [1] > > > >> > >> Let's add a new parameter 'const char *modname' to function > >> module_kallsyms_on_each_symbol(), then we can compare the module names > > > > we have use case for iterating all modules and their symbols when we > > want to resolve passed addresses for tracing > > > > we don't have 'modname' that we could pass, we need to iterate all modules > > > > so perhaps this could be made optional like with passing NULL for modname? > > The deletion of modname was suggested by Petr Mladek. The reason is that > everyone passes modname as NULL, there was no actual demand at the time. > https://lkml.org/lkml/2022/9/20/682 > > > > >> directly in this function and call hook 'fn' after matching. And the > >> parameter 'struct module *' in the hook 'fn' can also be deleted. > > > > we need 'struct module *' argument in the callback as well because we are > > taking the module reference if we trace function in it, so it wont get > > unloaded > > > > please let me know if I should do the change or can help in any way > > It seems that we should take the module reference before invoking callback > and put it after it is called, without passing modname. we take the module ref only if we (callback) find the traced address in the module, we don't have the module object before jirka > > > > > thanks, > > jirka > > > > [1] https://lore.kernel.org/lkml/20221114111350.38e44eec@canb.auug.org.au/ > > > >> > >> Phase1: mod1-->mod2..(subsequent modules do not need to be compared) > >> | > >> Phase2: -->f1-->f2-->f3 > >> > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > >> --- > >> include/linux/module.h | 4 ++-- > >> kernel/livepatch/core.c | 13 ++----------- > >> kernel/module/kallsyms.c | 15 ++++++++++++--- > >> 3 files changed, 16 insertions(+), 16 deletions(-) > >> > >> diff --git a/include/linux/module.h b/include/linux/module.h > >> index ec61fb53979a92a..0a3b44ff885a48c 100644 > >> --- a/include/linux/module.h > >> +++ b/include/linux/module.h > >> @@ -879,8 +879,8 @@ static inline bool module_sig_ok(struct module *module) > >> } > >> #endif /* CONFIG_MODULE_SIG */ > >> > >> -int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, > >> - struct module *, unsigned long), > >> +int module_kallsyms_on_each_symbol(const char *modname, > >> + int (*fn)(void *, const char *, unsigned long), > >> void *data); > >> > >> #endif /* _LINUX_MODULE_H */ > >> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > >> index 50bfc3481a4ee38..d4fe2d1b0e562bc 100644 > >> --- a/kernel/livepatch/core.c > >> +++ b/kernel/livepatch/core.c > >> @@ -118,27 +118,19 @@ static struct klp_object *klp_find_object(struct klp_patch *patch, > >> } > >> > >> struct klp_find_arg { > >> - const char *objname; > >> const char *name; > >> unsigned long addr; > >> unsigned long count; > >> unsigned long pos; > >> }; > >> > >> -static int klp_find_callback(void *data, const char *name, > >> - struct module *mod, unsigned long addr) > >> +static int klp_find_callback(void *data, const char *name, unsigned long addr) > >> { > >> struct klp_find_arg *args = data; > >> > >> - if ((mod && !args->objname) || (!mod && args->objname)) > >> - return 0; > >> - > >> if (strcmp(args->name, name)) > >> return 0; > >> > >> - if (args->objname && strcmp(args->objname, mod->name)) > >> - return 0; > >> - > >> args->addr = addr; > >> args->count++; > >> > >> @@ -175,7 +167,6 @@ static int klp_find_object_symbol(const char *objname, const char *name, > >> unsigned long sympos, unsigned long *addr) > >> { > >> struct klp_find_arg args = { > >> - .objname = objname, > >> .name = name, > >> .addr = 0, > >> .count = 0, > >> @@ -183,7 +174,7 @@ static int klp_find_object_symbol(const char *objname, const char *name, > >> }; > >> > >> if (objname) > >> - module_kallsyms_on_each_symbol(klp_find_callback, &args); > >> + module_kallsyms_on_each_symbol(objname, klp_find_callback, &args); > >> else > >> kallsyms_on_each_match_symbol(klp_match_callback, name, &args); > >> > >> diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c > >> index f5c5c9175333df7..329cef573675d49 100644 > >> --- a/kernel/module/kallsyms.c > >> +++ b/kernel/module/kallsyms.c > >> @@ -495,8 +495,8 @@ unsigned long module_kallsyms_lookup_name(const char *name) > >> } > >> > >> #ifdef CONFIG_LIVEPATCH > >> -int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, > >> - struct module *, unsigned long), > >> +int module_kallsyms_on_each_symbol(const char *modname, > >> + int (*fn)(void *, const char *, unsigned long), > >> void *data) > >> { > >> struct module *mod; > >> @@ -510,6 +510,9 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, > >> if (mod->state == MODULE_STATE_UNFORMED) > >> continue; > >> > >> + if (strcmp(modname, mod->name)) > >> + continue; > >> + > >> /* Use rcu_dereference_sched() to remain compliant with the sparse tool */ > >> preempt_disable(); > >> kallsyms = rcu_dereference_sched(mod->kallsyms); > >> @@ -522,10 +525,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)); > >> + 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); > >> -- > >> 2.25.1 > >> > > . > > > > -- > Regards, > Zhen Lei
On 2022/11/14 17:27, Jiri Olsa wrote: > On Mon, Nov 14, 2022 at 04:50:25PM +0800, Leizhen (ThunderTown) wrote: >> >> On 2022/11/14 15:47, Jiri Olsa wrote: >>> On Wed, Nov 02, 2022 at 04:49:19PM +0800, 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. >>> hi, >>> sorry for delayed answer, I did not notice this until Stephen's email >>> about merge issue with recent bpf change [1] >>> >>>> Let's add a new parameter 'const char *modname' to function >>>> module_kallsyms_on_each_symbol(), then we can compare the module names >>> we have use case for iterating all modules and their symbols when we >>> want to resolve passed addresses for tracing >>> >>> we don't have 'modname' that we could pass, we need to iterate all modules >>> >>> so perhaps this could be made optional like with passing NULL for modname? >> The deletion of modname was suggested by Petr Mladek. The reason is that >> everyone passes modname as NULL, there was no actual demand at the time. >> https://lkml.org/lkml/2022/9/20/682 >> >>>> directly in this function and call hook 'fn' after matching. And the >>>> parameter 'struct module *' in the hook 'fn' can also be deleted. >>> we need 'struct module *' argument in the callback as well because we are >>> taking the module reference if we trace function in it, so it wont get >>> unloaded >>> >>> please let me know if I should do the change or can help in any way >> It seems that we should take the module reference before invoking callback >> and put it after it is called, without passing modname. > we take the module ref only if we (callback) find the traced address in > the module, we don't have the module object before > > jirka > Do it in function module_kallsyms_on_each_symbol()? But I just saw that mutex_lock(&module_mutex) protection is already provided in this function. So reference counting protection may not be required.
On Mon, Nov 14, 2022 at 06:00:38PM +0800, Leizhen (ThunderTown) wrote: > > > On 2022/11/14 17:27, Jiri Olsa wrote: > > On Mon, Nov 14, 2022 at 04:50:25PM +0800, Leizhen (ThunderTown) wrote: > >> > >> On 2022/11/14 15:47, Jiri Olsa wrote: > >>> On Wed, Nov 02, 2022 at 04:49:19PM +0800, 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. > >>> hi, > >>> sorry for delayed answer, I did not notice this until Stephen's email > >>> about merge issue with recent bpf change [1] > >>> > >>>> Let's add a new parameter 'const char *modname' to function > >>>> module_kallsyms_on_each_symbol(), then we can compare the module names > >>> we have use case for iterating all modules and their symbols when we > >>> want to resolve passed addresses for tracing > >>> > >>> we don't have 'modname' that we could pass, we need to iterate all modules > >>> > >>> so perhaps this could be made optional like with passing NULL for modname? > >> The deletion of modname was suggested by Petr Mladek. The reason is that > >> everyone passes modname as NULL, there was no actual demand at the time. > >> https://lkml.org/lkml/2022/9/20/682 > >> > >>>> directly in this function and call hook 'fn' after matching. And the > >>>> parameter 'struct module *' in the hook 'fn' can also be deleted. > >>> we need 'struct module *' argument in the callback as well because we are > >>> taking the module reference if we trace function in it, so it wont get > >>> unloaded > >>> > >>> please let me know if I should do the change or can help in any way > >> It seems that we should take the module reference before invoking callback > >> and put it after it is called, without passing modname. > > we take the module ref only if we (callback) find the traced address in > > the module, we don't have the module object before > > > > jirka > > > > Do it in function module_kallsyms_on_each_symbol()? > > But I just saw that mutex_lock(&module_mutex) protection is already > provided in this function. So reference counting protection may not > be required. we take the module ref so it won't unload even outside of the module_kallsyms_on_each_symbol function jirka
On 2022/11/14 18:31, Jiri Olsa wrote: > On Mon, Nov 14, 2022 at 06:00:38PM +0800, Leizhen (ThunderTown) wrote: >> >> >> On 2022/11/14 17:27, Jiri Olsa wrote: >>> On Mon, Nov 14, 2022 at 04:50:25PM +0800, Leizhen (ThunderTown) wrote: >>>> >>>> On 2022/11/14 15:47, Jiri Olsa wrote: >>>>> On Wed, Nov 02, 2022 at 04:49:19PM +0800, 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. >>>>> hi, >>>>> sorry for delayed answer, I did not notice this until Stephen's email >>>>> about merge issue with recent bpf change [1] >>>>> >>>>>> Let's add a new parameter 'const char *modname' to function >>>>>> module_kallsyms_on_each_symbol(), then we can compare the module names >>>>> we have use case for iterating all modules and their symbols when we >>>>> want to resolve passed addresses for tracing >>>>> >>>>> we don't have 'modname' that we could pass, we need to iterate all modules >>>>> >>>>> so perhaps this could be made optional like with passing NULL for modname? >>>> The deletion of modname was suggested by Petr Mladek. The reason is that >>>> everyone passes modname as NULL, there was no actual demand at the time. >>>> https://lkml.org/lkml/2022/9/20/682 >>>> >>>>>> directly in this function and call hook 'fn' after matching. And the >>>>>> parameter 'struct module *' in the hook 'fn' can also be deleted. >>>>> we need 'struct module *' argument in the callback as well because we are >>>>> taking the module reference if we trace function in it, so it wont get >>>>> unloaded >>>>> >>>>> please let me know if I should do the change or can help in any way >>>> It seems that we should take the module reference before invoking callback >>>> and put it after it is called, without passing modname. >>> we take the module ref only if we (callback) find the traced address in >>> the module, we don't have the module object before >>> >>> jirka >>> >> >> Do it in function module_kallsyms_on_each_symbol()? >> >> But I just saw that mutex_lock(&module_mutex) protection is already >> provided in this function. So reference counting protection may not >> be required. > > we take the module ref so it won't unload even outside of the > module_kallsyms_on_each_symbol function There's another way to do it, but it's more time consuming. struct module *__module_text_address(unsigned long addr); struct module *__module_address(unsigned long addr); Which way do you think is more appropriate? > > jirka > . >
On Mon, Nov 14, 2022 at 07:30:16PM +0800, Leizhen (ThunderTown) wrote: > > > On 2022/11/14 18:31, Jiri Olsa wrote: > > On Mon, Nov 14, 2022 at 06:00:38PM +0800, Leizhen (ThunderTown) wrote: > >> > >> > >> On 2022/11/14 17:27, Jiri Olsa wrote: > >>> On Mon, Nov 14, 2022 at 04:50:25PM +0800, Leizhen (ThunderTown) wrote: > >>>> > >>>> On 2022/11/14 15:47, Jiri Olsa wrote: > >>>>> On Wed, Nov 02, 2022 at 04:49:19PM +0800, 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. > >>>>> hi, > >>>>> sorry for delayed answer, I did not notice this until Stephen's email > >>>>> about merge issue with recent bpf change [1] > >>>>> > >>>>>> Let's add a new parameter 'const char *modname' to function > >>>>>> module_kallsyms_on_each_symbol(), then we can compare the module names > >>>>> we have use case for iterating all modules and their symbols when we > >>>>> want to resolve passed addresses for tracing > >>>>> > >>>>> we don't have 'modname' that we could pass, we need to iterate all modules > >>>>> > >>>>> so perhaps this could be made optional like with passing NULL for modname? > >>>> The deletion of modname was suggested by Petr Mladek. The reason is that > >>>> everyone passes modname as NULL, there was no actual demand at the time. > >>>> https://lkml.org/lkml/2022/9/20/682 > >>>> > >>>>>> directly in this function and call hook 'fn' after matching. And the > >>>>>> parameter 'struct module *' in the hook 'fn' can also be deleted. > >>>>> we need 'struct module *' argument in the callback as well because we are > >>>>> taking the module reference if we trace function in it, so it wont get > >>>>> unloaded > >>>>> > >>>>> please let me know if I should do the change or can help in any way > >>>> It seems that we should take the module reference before invoking callback > >>>> and put it after it is called, without passing modname. > >>> we take the module ref only if we (callback) find the traced address in > >>> the module, we don't have the module object before > >>> > >>> jirka > >>> > >> > >> Do it in function module_kallsyms_on_each_symbol()? > >> > >> But I just saw that mutex_lock(&module_mutex) protection is already > >> provided in this function. So reference counting protection may not > >> be required. > > > > we take the module ref so it won't unload even outside of the > > module_kallsyms_on_each_symbol function > > There's another way to do it, but it's more time consuming. > > struct module *__module_text_address(unsigned long addr); > struct module *__module_address(unsigned long addr); > > Which way do you think is more appropriate? I think it'd be best to keep the module argument in the callback, I don't see the harm.. AFAICS it was removed because you thought nobody was using it, but that's not the case using __module_text_address/__module_address might be doable and it might be fast enough, because it's using rbtree I'll check on that, meanwhile if we could keep the module argument, that'd be great thanks, jirka
On Mon, Nov 14, 2022 at 02:26:19PM +0100, Jiri Olsa wrote: > I'll check on that, meanwhile if we could keep the module argument, > that'd be great As Leizhen suggested I could just drop patches: 7/9 livepatch: Improve the search performance of module_kallsyms_on_each_symbol() 8/9 kallsyms: Delete an unused parameter related to kallsyms_on_each_symbol() Then after the next kernel is released this can be relooked at. If this is agreeable let me know. Luis
On 2022/11/14 23:46, Luis Chamberlain wrote: > On Mon, Nov 14, 2022 at 02:26:19PM +0100, Jiri Olsa wrote: >> I'll check on that, meanwhile if we could keep the module argument, >> that'd be great > > As Leizhen suggested I could just drop patches: > > 7/9 livepatch: Improve the search performance of module_kallsyms_on_each_symbol() > 8/9 kallsyms: Delete an unused parameter related to kallsyms_on_each_symbol() > > Then after the next kernel is released this can be relooked at. > If this is agreeable let me know. I'm OK. > > Luis > . >
On Tue, Nov 15, 2022 at 10:10:16AM +0800, Leizhen (ThunderTown) wrote: > > > On 2022/11/14 23:46, Luis Chamberlain wrote: > > On Mon, Nov 14, 2022 at 02:26:19PM +0100, Jiri Olsa wrote: > >> I'll check on that, meanwhile if we could keep the module argument, > >> that'd be great > > > > As Leizhen suggested I could just drop patches: > > > > 7/9 livepatch: Improve the search performance of module_kallsyms_on_each_symbol() > > 8/9 kallsyms: Delete an unused parameter related to kallsyms_on_each_symbol() > > > > Then after the next kernel is released this can be relooked at. > > If this is agreeable let me know. > > I'm OK. sounds good, thanks jirka
On Tue, Nov 15, 2022 at 08:30:23AM +0100, Jiri Olsa wrote: > On Tue, Nov 15, 2022 at 10:10:16AM +0800, Leizhen (ThunderTown) wrote: > > > > > > On 2022/11/14 23:46, Luis Chamberlain wrote: > > > On Mon, Nov 14, 2022 at 02:26:19PM +0100, Jiri Olsa wrote: > > >> I'll check on that, meanwhile if we could keep the module argument, > > >> that'd be great > > > > > > As Leizhen suggested I could just drop patches: > > > > > > 7/9 livepatch: Improve the search performance of module_kallsyms_on_each_symbol() > > > 8/9 kallsyms: Delete an unused parameter related to kallsyms_on_each_symbol() > > > > > > Then after the next kernel is released this can be relooked at. > > > If this is agreeable let me know. > > > > I'm OK. > > sounds good, thanks OK thanks, I dropped the last selftest patch as well, and pushed to modules-next. Leizhen, can you enhance the selftest for the new module requirement and repost? Stephen, you can drop your fix from linux-next, hopefully there should no longer be any merge conflicts. The module requirement will stick for now. Thanks, Luis
On 2022/11/15 15:54, Luis Chamberlain wrote: > On Tue, Nov 15, 2022 at 08:30:23AM +0100, Jiri Olsa wrote: >> On Tue, Nov 15, 2022 at 10:10:16AM +0800, Leizhen (ThunderTown) wrote: >>> >>> >>> On 2022/11/14 23:46, Luis Chamberlain wrote: >>>> On Mon, Nov 14, 2022 at 02:26:19PM +0100, Jiri Olsa wrote: >>>>> I'll check on that, meanwhile if we could keep the module argument, >>>>> that'd be great >>>> >>>> As Leizhen suggested I could just drop patches: >>>> >>>> 7/9 livepatch: Improve the search performance of module_kallsyms_on_each_symbol() >>>> 8/9 kallsyms: Delete an unused parameter related to kallsyms_on_each_symbol() >>>> >>>> Then after the next kernel is released this can be relooked at. >>>> If this is agreeable let me know. >>> >>> I'm OK. >> >> sounds good, thanks > > OK thanks, I dropped the last selftest patch as well, and pushed to > modules-next. Leizhen, can you enhance the selftest for the new module > requirement and repost? Okay, right away. > > Stephen, you can drop your fix from linux-next, hopefully there should > no longer be any merge conflicts. The module requirement will stick for > now. > > Thanks, > > Luis > . >
Hi Luis, On Mon, 14 Nov 2022 23:54:08 -0800 Luis Chamberlain <mcgrof@kernel.org> wrote: > > Stephen, you can drop your fix from linux-next, hopefully there should > no longer be any merge conflicts. The module requirement will stick for > now. OK, will do.
On Wed 2022-11-02 16:49:19, 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. > > Let's add a new parameter 'const char *modname' to function > module_kallsyms_on_each_symbol(), then we can compare the module names > directly in this function and call hook 'fn' after matching. And the > parameter 'struct module *' in the hook 'fn' can also be deleted. > > Phase1: mod1-->mod2..(subsequent modules do not need to be compared) > | > Phase2: -->f1-->f2-->f3 Just for record. The patch looks good from the livepatching code POV. But I guess that it will need to get updated to support the new callers in the ftrace and bpf code. Best Regards, Petr
On 2022/11/23 21:57, Petr Mladek wrote: > On Wed 2022-11-02 16:49:19, 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. >> >> Let's add a new parameter 'const char *modname' to function >> module_kallsyms_on_each_symbol(), then we can compare the module names >> directly in this function and call hook 'fn' after matching. And the >> parameter 'struct module *' in the hook 'fn' can also be deleted. >> >> Phase1: mod1-->mod2..(subsequent modules do not need to be compared) >> | >> Phase2: -->f1-->f2-->f3 > > Just for record. The patch looks good from the livepatching code POV. > > But I guess that it will need to get updated to support the new > callers in the ftrace and bpf code. Yes, this patch and 8/9 conflicts with the patch series of Jiri Olsa, So they didn't merge into linux-next. I will update and repost after v6.2-rc1. > > Best Regards, > Petr > . >
diff --git a/include/linux/module.h b/include/linux/module.h index ec61fb53979a92a..0a3b44ff885a48c 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -879,8 +879,8 @@ static inline bool module_sig_ok(struct module *module) } #endif /* CONFIG_MODULE_SIG */ -int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, - struct module *, unsigned long), +int module_kallsyms_on_each_symbol(const char *modname, + int (*fn)(void *, const char *, unsigned long), void *data); #endif /* _LINUX_MODULE_H */ diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 50bfc3481a4ee38..d4fe2d1b0e562bc 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -118,27 +118,19 @@ static struct klp_object *klp_find_object(struct klp_patch *patch, } struct klp_find_arg { - const char *objname; const char *name; unsigned long addr; unsigned long count; unsigned long pos; }; -static int klp_find_callback(void *data, const char *name, - struct module *mod, unsigned long addr) +static int klp_find_callback(void *data, const char *name, unsigned long addr) { struct klp_find_arg *args = data; - if ((mod && !args->objname) || (!mod && args->objname)) - return 0; - if (strcmp(args->name, name)) return 0; - if (args->objname && strcmp(args->objname, mod->name)) - return 0; - args->addr = addr; args->count++; @@ -175,7 +167,6 @@ static int klp_find_object_symbol(const char *objname, const char *name, unsigned long sympos, unsigned long *addr) { struct klp_find_arg args = { - .objname = objname, .name = name, .addr = 0, .count = 0, @@ -183,7 +174,7 @@ static int klp_find_object_symbol(const char *objname, const char *name, }; if (objname) - module_kallsyms_on_each_symbol(klp_find_callback, &args); + module_kallsyms_on_each_symbol(objname, klp_find_callback, &args); else kallsyms_on_each_match_symbol(klp_match_callback, name, &args); diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c index f5c5c9175333df7..329cef573675d49 100644 --- a/kernel/module/kallsyms.c +++ b/kernel/module/kallsyms.c @@ -495,8 +495,8 @@ unsigned long module_kallsyms_lookup_name(const char *name) } #ifdef CONFIG_LIVEPATCH -int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, - struct module *, unsigned long), +int module_kallsyms_on_each_symbol(const char *modname, + int (*fn)(void *, const char *, unsigned long), void *data) { struct module *mod; @@ -510,6 +510,9 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, if (mod->state == MODULE_STATE_UNFORMED) continue; + if (strcmp(modname, mod->name)) + continue; + /* Use rcu_dereference_sched() to remain compliant with the sparse tool */ preempt_disable(); kallsyms = rcu_dereference_sched(mod->kallsyms); @@ -522,10 +525,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)); + 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. Let's add a new parameter 'const char *modname' to function module_kallsyms_on_each_symbol(), then we can compare the module names directly in this function and call hook 'fn' after matching. And the parameter 'struct module *' in the hook 'fn' can also be deleted. Phase1: mod1-->mod2..(subsequent modules do not need to be compared) | Phase2: -->f1-->f2-->f3 Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- include/linux/module.h | 4 ++-- kernel/livepatch/core.c | 13 ++----------- kernel/module/kallsyms.c | 15 ++++++++++++--- 3 files changed, 16 insertions(+), 16 deletions(-)