Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases
On 2017/04/27 11:21AM, Masami Hiramatsu wrote: > On Thu, 27 Apr 2017 01:38:10 +0530 > "Naveen N. Rao" <naveen.n@linux.vnet.ibm.com> wrote: > > > Michael Ellerman wrote: > > > "Naveen N. Rao" <naveen.n@linux.vnet.ibm.com> writes: > > >> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > > >> index 6a3b249a2ae1..d134b060564f 100644 > > >> --- a/kernel/kallsyms.c > > >> +++ b/kernel/kallsyms.c > > >> @@ -205,6 +205,12 @@ unsigned long kallsyms_lookup_name(const char *name) > > >> unsigned long i; > > >> unsigned int off; > > >> > > >> +if (!name || *name == '\0') > > >> +return false; > > >> + > > >> +if (strnchr(name, MODULE_NAME_LEN, ':')) > > >> +return module_kallsyms_lookup_name(name); > > >> + > > >> for (i = 0, off = 0; i < kallsyms_num_syms; i++) { > > >> off = kallsyms_expand_symbol(off, namebuf, > > >> ARRAY_SIZE(namebuf)); > > > ... > > > } > > > return module_kallsyms_lookup_name(name); > > > > > > Is the rest of the context. > > > > > > Which looks a bit odd, we already did module lookup previously? > > > > > > But it's correct, because you can lookup a symbol in a module without a > > > module prefix, it just looks in every module. > > > > Yes. > > > > > > > > You could invert the logic, ie. check that there isn't a ":" in the name > > > and only in that case do the for loop, always falling back to module > > > lookup. > > > > > > Or just add a comment explaining why we call module lookup in two places. > > > > Good point. Here's a v2 - I'm using a goto so as to not indent the code too > > much. > > > > Thanks for the review! > > - Naveen > > > > -- > > [PATCH v2] kallsyms: optimize kallsyms_lookup_name() for a few cases > > > > 1. Fail early for invalid/zero length symbols. > > 2. Detect names of the form and skip checking for kernel > > symbols in that case. > > > > Looks good to me. > > Reviewed-by: Masami Hiramatsu <mhira...@kernel.org> Thanks, Masami! I am not quite sure who maintains kallsyms... Ingo, Can you please help with merging this patch? Thanks, Naveen > > Thanks, > > > > Signed-off-by: Naveen N. Rao <naveen.n@linux.vnet.ibm.com> > > --- > > kernel/kallsyms.c | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > > index 6a3b249a2ae1..f7558dc5c6ac 100644 > > --- a/kernel/kallsyms.c > > +++ b/kernel/kallsyms.c > > @@ -205,12 +205,20 @@ unsigned long kallsyms_lookup_name(const char *name) > > unsigned long i; > > unsigned int off; > > > > + if (!name || *name == '\0') > > + return 0; > > + > > + /* For symbols of the form :, only check the modules */ > > + if (strnchr(name, MODULE_NAME_LEN, ':')) > > + goto mod; > > + > > for (i = 0, off = 0; i < kallsyms_num_syms; i++) { > > off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); > > > > if (strcmp(namebuf, name) == 0) > > return kallsyms_sym_address(i); > > } > > +mod: > > return module_kallsyms_lookup_name(name); > > } > > EXPORT_SYMBOL_GPL(kallsyms_lookup_name); > > -- > > 2.12.2 > > > > > -- > Masami Hiramatsu <mhira...@kernel.org> >
Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases
On Thu, 27 Apr 2017 01:38:10 +0530 "Naveen N. Rao" <naveen.n@linux.vnet.ibm.com> wrote: > Michael Ellerman wrote: > > "Naveen N. Rao" <naveen.n@linux.vnet.ibm.com> writes: > >> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > >> index 6a3b249a2ae1..d134b060564f 100644 > >> --- a/kernel/kallsyms.c > >> +++ b/kernel/kallsyms.c > >> @@ -205,6 +205,12 @@ unsigned long kallsyms_lookup_name(const char *name) > >>unsigned long i; > >>unsigned int off; > >> > >> + if (!name || *name == '\0') > >> + return false; > >> + > >> + if (strnchr(name, MODULE_NAME_LEN, ':')) > >> + return module_kallsyms_lookup_name(name); > >> + > >>for (i = 0, off = 0; i < kallsyms_num_syms; i++) { > >>off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); > > ... > > } > > return module_kallsyms_lookup_name(name); > > > > Is the rest of the context. > > > > Which looks a bit odd, we already did module lookup previously? > > > > But it's correct, because you can lookup a symbol in a module without a > > module prefix, it just looks in every module. > > Yes. > > > > > You could invert the logic, ie. check that there isn't a ":" in the name > > and only in that case do the for loop, always falling back to module > > lookup. > > > > Or just add a comment explaining why we call module lookup in two places. > > Good point. Here's a v2 - I'm using a goto so as to not indent the code too > much. > > Thanks for the review! > - Naveen > > -- > [PATCH v2] kallsyms: optimize kallsyms_lookup_name() for a few cases > > 1. Fail early for invalid/zero length symbols. > 2. Detect names of the form and skip checking for kernel > symbols in that case. > Looks good to me. Reviewed-by: Masami Hiramatsu <mhira...@kernel.org> Thanks, > Signed-off-by: Naveen N. Rao <naveen.n@linux.vnet.ibm.com> > --- > kernel/kallsyms.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > index 6a3b249a2ae1..f7558dc5c6ac 100644 > --- a/kernel/kallsyms.c > +++ b/kernel/kallsyms.c > @@ -205,12 +205,20 @@ unsigned long kallsyms_lookup_name(const char *name) > unsigned long i; > unsigned int off; > > + if (!name || *name == '\0') > + return 0; > + > + /* For symbols of the form :, only check the modules */ > + if (strnchr(name, MODULE_NAME_LEN, ':')) > + goto mod; > + > for (i = 0, off = 0; i < kallsyms_num_syms; i++) { > off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); > > if (strcmp(namebuf, name) == 0) > return kallsyms_sym_address(i); > } > +mod: > return module_kallsyms_lookup_name(name); > } > EXPORT_SYMBOL_GPL(kallsyms_lookup_name); > -- > 2.12.2 > -- Masami Hiramatsu <mhira...@kernel.org>
Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases
Michael Ellerman wrote: > "Naveen N. Rao" <naveen.n@linux.vnet.ibm.com> writes: >> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c >> index 6a3b249a2ae1..d134b060564f 100644 >> --- a/kernel/kallsyms.c >> +++ b/kernel/kallsyms.c >> @@ -205,6 +205,12 @@ unsigned long kallsyms_lookup_name(const char *name) >> unsigned long i; >> unsigned int off; >> >> +if (!name || *name == '\0') >> +return false; >> + >> +if (strnchr(name, MODULE_NAME_LEN, ':')) >> +return module_kallsyms_lookup_name(name); >> + >> for (i = 0, off = 0; i < kallsyms_num_syms; i++) { >> off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); > ... > } > return module_kallsyms_lookup_name(name); > > Is the rest of the context. > > Which looks a bit odd, we already did module lookup previously? > > But it's correct, because you can lookup a symbol in a module without a > module prefix, it just looks in every module. Yes. > > You could invert the logic, ie. check that there isn't a ":" in the name > and only in that case do the for loop, always falling back to module > lookup. > > Or just add a comment explaining why we call module lookup in two places. Good point. Here's a v2 - I'm using a goto so as to not indent the code too much. Thanks for the review! - Naveen -- [PATCH v2] kallsyms: optimize kallsyms_lookup_name() for a few cases 1. Fail early for invalid/zero length symbols. 2. Detect names of the form and skip checking for kernel symbols in that case. Signed-off-by: Naveen N. Rao <naveen.n@linux.vnet.ibm.com> --- kernel/kallsyms.c | 8 1 file changed, 8 insertions(+) diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 6a3b249a2ae1..f7558dc5c6ac 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -205,12 +205,20 @@ unsigned long kallsyms_lookup_name(const char *name) unsigned long i; unsigned int off; + if (!name || *name == '\0') + return 0; + + /* For symbols of the form :, only check the modules */ + if (strnchr(name, MODULE_NAME_LEN, ':')) + goto mod; + for (i = 0, off = 0; i < kallsyms_num_syms; i++) { off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); if (strcmp(namebuf, name) == 0) return kallsyms_sym_address(i); } +mod: return module_kallsyms_lookup_name(name); } EXPORT_SYMBOL_GPL(kallsyms_lookup_name); -- 2.12.2
Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases
"Naveen N. Rao"writes: > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > index 6a3b249a2ae1..d134b060564f 100644 > --- a/kernel/kallsyms.c > +++ b/kernel/kallsyms.c > @@ -205,6 +205,12 @@ unsigned long kallsyms_lookup_name(const char *name) > unsigned long i; > unsigned int off; > > + if (!name || *name == '\0') > + return false; > + > + if (strnchr(name, MODULE_NAME_LEN, ':')) > + return module_kallsyms_lookup_name(name); > + > for (i = 0, off = 0; i < kallsyms_num_syms; i++) { > off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); ... } return module_kallsyms_lookup_name(name); Is the rest of the context. Which looks a bit odd, we already did module lookup previously? But it's correct, because you can lookup a symbol in a module without a module prefix, it just looks in every module. You could invert the logic, ie. check that there isn't a ":" in the name and only in that case do the for loop, always falling back to module lookup. Or just add a comment explaining why we call module lookup in two places. cheers
RE: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases
Excerpts from David Laight's message of April 25, 2017 22:06: From: Naveen N. Rao Sent: 25 April 2017 17:18 1. Fail early for invalid/zero length symbols. 2. Detect names of the form and skip checking for kernel symbols in that case. Signed-off-by: Naveen N. Rao--- Masami, Michael, I have added two very simple checks here, which I felt is good to have, rather than the elaborate checks in the previous version. Given the change in module code to use strnchr(), the checks are now safe and further tests are not probably not that useful. ... + if (strnchr(name, MODULE_NAME_LEN, ':')) + return module_kallsyms_lookup_name(name); Should that be MODULE_NAME_LEN - 1 ? The ':' character _follows_ the module name, which can itself be upto MODULE_NAME_LEN - 1 characters. So, we should look for it till MODULE_NAME_LEN. Thanks for the review, - Naveen
RE: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases
From: Naveen N. Rao > Sent: 25 April 2017 17:18 > 1. Fail early for invalid/zero length symbols. > 2. Detect names of the form and skip checking for kernel > symbols in that case. > > Signed-off-by: Naveen N. Rao> --- > Masami, Michael, > I have added two very simple checks here, which I felt is good to have, > rather than the elaborate checks in the previous version. Given the > change in module code to use strnchr(), the checks are now safe and > further tests are not probably not that useful. ... > + if (strnchr(name, MODULE_NAME_LEN, ':')) > + return module_kallsyms_lookup_name(name); Should that be MODULE_NAME_LEN - 1 ? David
[PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases
1. Fail early for invalid/zero length symbols. 2. Detect names of the form and skip checking for kernel symbols in that case. Signed-off-by: Naveen N. Rao--- Masami, Michael, I have added two very simple checks here, which I felt is good to have, rather than the elaborate checks in the previous version. Given the change in module code to use strnchr(), the checks are now safe and further tests are not probably not that useful. - Naveen kernel/kallsyms.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 6a3b249a2ae1..d134b060564f 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -205,6 +205,12 @@ unsigned long kallsyms_lookup_name(const char *name) unsigned long i; unsigned int off; + if (!name || *name == '\0') + return false; + + if (strnchr(name, MODULE_NAME_LEN, ':')) + return module_kallsyms_lookup_name(name); + for (i = 0, off = 0; i < kallsyms_num_syms; i++) { off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); -- 2.12.1