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@xxxxxxxxxxxxxxxxxx> wrote:

> Michael Ellerman wrote:
> > "Naveen N. Rao" <naveen.n.rao@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>

Thanks,


> Signed-off-by: Naveen N. Rao <naveen.n.rao@xxxxxxxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>