* [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases @ 2017-04-25 16:18 Naveen N. Rao 2017-04-25 16:36 ` David Laight 2017-04-26 10:44 ` Michael Ellerman 0 siblings, 2 replies; 7+ messages in thread From: Naveen N. Rao @ 2017-04-25 16:18 UTC (permalink / raw) To: Michael Ellerman Cc: Masami Hiramatsu, Paul Clarke, David Laight, linux-kernel, linuxppc-dev 1. Fail early for invalid/zero length symbols. 2. Detect names of the form <mod:name> and skip checking for kernel symbols in that case. Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases 2017-04-25 16:18 [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases Naveen N. Rao @ 2017-04-25 16:36 ` David Laight 2017-04-25 17:17 ` Naveen N. Rao 2017-04-26 10:44 ` Michael Ellerman 1 sibling, 1 reply; 7+ messages in thread From: David Laight @ 2017-04-25 16:36 UTC (permalink / raw) To: 'Naveen N. Rao', Michael Ellerman Cc: Masami Hiramatsu, Paul Clarke, linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org From: Naveen N. Rao > Sent: 25 April 2017 17:18 > 1. Fail early for invalid/zero length symbols. > 2. Detect names of the form <mod:name> and skip checking for kernel > symbols in that case. >=20 > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > --- > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases 2017-04-25 16:36 ` David Laight @ 2017-04-25 17:17 ` Naveen N. Rao 0 siblings, 0 replies; 7+ messages in thread From: Naveen N. Rao @ 2017-04-25 17:17 UTC (permalink / raw) To: David Laight, Michael Ellerman Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, Masami Hiramatsu, Paul Clarke 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 <mod:name> and skip checking for kernel >> symbols in that case. >>=20 >> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> >> --- >> 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); >=20 > Should that be MODULE_NAME_LEN - 1 ? The ':' character _follows_ the module name, which can itself be upto=20 MODULE_NAME_LEN - 1 characters. So, we should look for it till=20 MODULE_NAME_LEN. Thanks for the review, - Naveen = ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases 2017-04-25 16:18 [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases Naveen N. Rao 2017-04-25 16:36 ` David Laight @ 2017-04-26 10:44 ` Michael Ellerman 2017-04-26 20:08 ` Naveen N. Rao 1 sibling, 1 reply; 7+ messages in thread From: Michael Ellerman @ 2017-04-26 10:44 UTC (permalink / raw) To: Naveen N. Rao Cc: Masami Hiramatsu, Paul Clarke, David Laight, linux-kernel, linuxppc-dev "Naveen N. Rao" <naveen.n.rao@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. 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases 2017-04-26 10:44 ` Michael Ellerman @ 2017-04-26 20:08 ` Naveen N. Rao 2017-04-27 2:21 ` Masami Hiramatsu 0 siblings, 1 reply; 7+ messages in thread From: Naveen N. Rao @ 2017-04-26 20:08 UTC (permalink / raw) To: Michael Ellerman Cc: David Laight, Masami Hiramatsu, Paul Clarke, linuxppc-dev, linux-kernel Michael Ellerman wrote: > "Naveen N. Rao" <naveen.n.rao@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 <mod:name> and skip checking for kernel symbols in that case. Signed-off-by: Naveen N. Rao <naveen.n.rao@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 <mod>:<sym>, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases 2017-04-26 20:08 ` Naveen N. Rao @ 2017-04-27 2:21 ` Masami Hiramatsu 2017-06-27 9:28 ` Naveen N. Rao 0 siblings, 1 reply; 7+ messages in thread From: Masami Hiramatsu @ 2017-04-27 2:21 UTC (permalink / raw) To: Naveen N. Rao Cc: Michael Ellerman, David Laight, Masami Hiramatsu, Paul Clarke, linuxppc-dev, linux-kernel On Thu, 27 Apr 2017 01:38:10 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > Michael Ellerman wrote: > > "Naveen N. Rao" <naveen.n.rao@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 <mod:name> and skip checking for kernel > symbols in that case. > Looks good to me. Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> Thanks, > Signed-off-by: Naveen N. Rao <naveen.n.rao@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 <mod>:<sym>, 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 <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases 2017-04-27 2:21 ` Masami Hiramatsu @ 2017-06-27 9:28 ` Naveen N. Rao 0 siblings, 0 replies; 7+ messages in thread From: Naveen N. Rao @ 2017-06-27 9:28 UTC (permalink / raw) To: Masami Hiramatsu, Ingo Molnar Cc: linux-kernel, David Laight, linuxppc-dev, Paul Clarke On 2017/04/27 11:21AM, Masami Hiramatsu wrote: > On Thu, 27 Apr 2017 01:38:10 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > > Michael Ellerman wrote: > > > "Naveen N. Rao" <naveen.n.rao@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 <mod:name> and skip checking for kernel > > symbols in that case. > > > > Looks good to me. > > Reviewed-by: Masami Hiramatsu <mhiramat@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.rao@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 <mod>:<sym>, 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 <mhiramat@kernel.org> > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-06-27 9:28 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-04-25 16:18 [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases Naveen N. Rao 2017-04-25 16:36 ` David Laight 2017-04-25 17:17 ` Naveen N. Rao 2017-04-26 10:44 ` Michael Ellerman 2017-04-26 20:08 ` Naveen N. Rao 2017-04-27 2:21 ` Masami Hiramatsu 2017-06-27 9:28 ` Naveen N. Rao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).