[PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases

[PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases

[Naveen N. Rao]

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

RE: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases

[David Laight]

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.
> 
> 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

RE: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases

[Naveen N. Rao]

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.
>> 
>> 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 ?

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

[Michael Ellerman]

"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

Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases

[Naveen N. Rao]

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

Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases

[Masami Hiramatsu]

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>

Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases

[Naveen N. Rao]

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>
>