Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630743 > unrolled thread

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

Started by"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
First post2017-04-25 18:20 +0200
Last post2017-04-27 04:30 +0200
Articles 6 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-04-25 18:20 +0200
    RE: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few  cases David Laight <David.Laight@ACULAB.COM> - 2017-04-25 18:40 +0200
      RE: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-04-25 19:20 +0200
    Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases Michael Ellerman <mpe@ellerman.id.au> - 2017-04-26 12:50 +0200
      Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-04-26 22:10 +0200
        Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few  cases Masami Hiramatsu <mhiramat@kernel.org> - 2017-04-27 04:30 +0200

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

From"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Date2017-04-25 18:20 +0200
Subject[PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases
Message-ID<tAbm2-70X-23@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

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

FromDavid Laight <David.Laight@ACULAB.COM>
Date2017-04-25 18:40 +0200
SubjectRE: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases
Message-ID<tAbFo-77u-25@gated-at.bofh.it>
In reply to#1630743
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

[toc] | [prev] | [next] | [standalone]

#1630815

From"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Date2017-04-25 19:20 +0200
Message-ID<tAci5-7A0-9@gated-at.bofh.it>
In reply to#1630764
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

[toc] | [prev] | [next] | [standalone]

#1631371

FromMichael Ellerman <mpe@ellerman.id.au>
Date2017-04-26 12:50 +0200
Message-ID<tAsGe-15h-11@gated-at.bofh.it>
In reply to#1630743
"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

[toc] | [prev] | [next] | [standalone]

#1631699

From"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Date2017-04-26 22:10 +0200
Message-ID<tABqa-73J-13@gated-at.bofh.it>
In reply to#1631371
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

[toc] | [prev] | [next] | [standalone]

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

FromMasami Hiramatsu <mhiramat@kernel.org>
Date2017-04-27 04:30 +0200
SubjectRe: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases
Message-ID<tAHlT-2zN-1@gated-at.bofh.it>
In reply to#1631699
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>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web