| Message ID | 20221102084921.1615-7-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kallsyms: Optimizes the performance of lookup symbols | expand |
Hi, I am sorry for the late review. I have been snowed under another tasks. On Wed 2022-11-02 16:49:18, Zhen Lei wrote: > Based on the test results of kallsyms_on_each_match_symbol() and > kallsyms_on_each_symbol(), the average performance can be improved by > more than 1500 times. Sounds great. > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -153,6 +153,24 @@ static int klp_find_callback(void *data, const char *name, > return 0; > } > > +static int klp_match_callback(void *data, unsigned long addr) > +{ > + struct klp_find_arg *args = data; > + > + 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; This duplicates most of the klp_find_callback(). Please, call this new function in klp_find_callback() instead of the duplicated code. I mean to do: static int klp_find_callback(void *data, const char *name, unsigned long addr) { struct klp_find_arg *args = data; if (strcmp(args->name, name)) return 0; return klp_match_callback(data, addr); } Otherwise, it looks good. Best Regards, Petr
On 2022/11/23 21:28, Petr Mladek wrote: > Hi, > > I am sorry for the late review. I have been snowed under another > tasks. > > On Wed 2022-11-02 16:49:18, Zhen Lei wrote: >> Based on the test results of kallsyms_on_each_match_symbol() and >> kallsyms_on_each_symbol(), the average performance can be improved by >> more than 1500 times. > > Sounds great. > >> --- a/kernel/livepatch/core.c >> +++ b/kernel/livepatch/core.c >> @@ -153,6 +153,24 @@ static int klp_find_callback(void *data, const char *name, >> return 0; >> } >> >> +static int klp_match_callback(void *data, unsigned long addr) >> +{ >> + struct klp_find_arg *args = data; >> + >> + 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; > > This duplicates most of the klp_find_callback(). Please, call this > new function in klp_find_callback() instead of the duplicated code. > I mean to do: > > static int klp_find_callback(void *data, const char *name, unsigned long addr) > { > struct klp_find_arg *args = data; > > if (strcmp(args->name, name)) > return 0; > > return klp_match_callback(data, addr); > } Good idea. But these patches have been merged into linux-next, how about I post a new cleanup patch after v6.2-rc1? > > Otherwise, it looks good. > > Best Regards, > Petr > . >
On Thu 2022-11-24 10:36:23, Leizhen (ThunderTown) wrote: > On 2022/11/23 21:28, Petr Mladek wrote: > > Hi, > > > > I am sorry for the late review. I have been snowed under another > > tasks. > > > > On Wed 2022-11-02 16:49:18, Zhen Lei wrote: > >> Based on the test results of kallsyms_on_each_match_symbol() and > >> kallsyms_on_each_symbol(), the average performance can be improved by > >> more than 1500 times. > > > > Sounds great. > > > >> --- a/kernel/livepatch/core.c > >> +++ b/kernel/livepatch/core.c > >> @@ -153,6 +153,24 @@ static int klp_find_callback(void *data, const char *name, > >> return 0; > >> } > >> > >> +static int klp_match_callback(void *data, unsigned long addr) > >> +{ > >> + struct klp_find_arg *args = data; > >> + > >> + 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; > > > > This duplicates most of the klp_find_callback(). Please, call this > > new function in klp_find_callback() instead of the duplicated code. > > I mean to do: > > > > static int klp_find_callback(void *data, const char *name, unsigned long addr) > > { > > struct klp_find_arg *args = data; > > > > if (strcmp(args->name, name)) > > return 0; > > > > return klp_match_callback(data, addr); > > } > > Good idea. But these patches have been merged into linux-next, how about I post > a new cleanup patch after v6.2-rc1? I am fine with it. Best Regards, Petr
On Thu, Nov 24, 2022 at 10:36:23AM +0800, Leizhen (ThunderTown) wrote: > > > On 2022/11/23 21:28, Petr Mladek wrote: > > Hi, > > > > I am sorry for the late review. I have been snowed under another > > tasks. > > > > On Wed 2022-11-02 16:49:18, Zhen Lei wrote: > >> Based on the test results of kallsyms_on_each_match_symbol() and > >> kallsyms_on_each_symbol(), the average performance can be improved by > >> more than 1500 times. > > > > Sounds great. > > > >> --- a/kernel/livepatch/core.c > >> +++ b/kernel/livepatch/core.c > >> @@ -153,6 +153,24 @@ static int klp_find_callback(void *data, const char *name, > >> return 0; > >> } > >> > >> +static int klp_match_callback(void *data, unsigned long addr) > >> +{ > >> + struct klp_find_arg *args = data; > >> + > >> + 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; > > > > This duplicates most of the klp_find_callback(). Please, call this > > new function in klp_find_callback() instead of the duplicated code. > > I mean to do: > > > > static int klp_find_callback(void *data, const char *name, unsigned long addr) > > { > > struct klp_find_arg *args = data; > > > > if (strcmp(args->name, name)) > > return 0; > > > > return klp_match_callback(data, addr); > > } > > Good idea. But these patches have been merged into linux-next, how about I post > a new cleanup patch after v6.2-rc1? You can send the cleanup now. The code doesn't change drastically, just base it on modules-next. Luis
On 2022/12/7 6:08, Luis Chamberlain wrote: > On Thu, Nov 24, 2022 at 10:36:23AM +0800, Leizhen (ThunderTown) wrote: >> >> >> On 2022/11/23 21:28, Petr Mladek wrote: >>> Hi, >>> >>> I am sorry for the late review. I have been snowed under another >>> tasks. >>> >>> On Wed 2022-11-02 16:49:18, Zhen Lei wrote: >>>> Based on the test results of kallsyms_on_each_match_symbol() and >>>> kallsyms_on_each_symbol(), the average performance can be improved by >>>> more than 1500 times. >>> >>> Sounds great. >>> >>>> --- a/kernel/livepatch/core.c >>>> +++ b/kernel/livepatch/core.c >>>> @@ -153,6 +153,24 @@ static int klp_find_callback(void *data, const char *name, >>>> return 0; >>>> } >>>> >>>> +static int klp_match_callback(void *data, unsigned long addr) >>>> +{ >>>> + struct klp_find_arg *args = data; >>>> + >>>> + 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; >>> >>> This duplicates most of the klp_find_callback(). Please, call this >>> new function in klp_find_callback() instead of the duplicated code. >>> I mean to do: >>> >>> static int klp_find_callback(void *data, const char *name, unsigned long addr) >>> { >>> struct klp_find_arg *args = data; >>> >>> if (strcmp(args->name, name)) >>> return 0; >>> >>> return klp_match_callback(data, addr); >>> } >> >> Good idea. But these patches have been merged into linux-next, how about I post >> a new cleanup patch after v6.2-rc1? > > You can send the cleanup now. The code doesn't change drastically, just > base it on modules-next. OK > > Luis > . >
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 9ada0bc5247be5d..50bfc3481a4ee38 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -153,6 +153,24 @@ static int klp_find_callback(void *data, const char *name, return 0; } +static int klp_match_callback(void *data, unsigned long addr) +{ + struct klp_find_arg *args = data; + + 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; +} + static int klp_find_object_symbol(const char *objname, const char *name, unsigned long sympos, unsigned long *addr) { @@ -167,7 +185,7 @@ static int klp_find_object_symbol(const char *objname, const char *name, if (objname) module_kallsyms_on_each_symbol(klp_find_callback, &args); else - kallsyms_on_each_symbol(klp_find_callback, &args); + kallsyms_on_each_match_symbol(klp_match_callback, name, &args); /* * Ensure an address was found. If sympos is 0, ensure symbol is unique;
Based on the test results of kallsyms_on_each_match_symbol() and kallsyms_on_each_symbol(), the average performance can be improved by more than 1500 times. Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- kernel/livepatch/core.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)