[RFC/PATCH] init/main.c: Simplify initcall_blacklisted()

[RFC/PATCH] init/main.c: Simplify initcall_blacklisted()

[Rasmus Villemoes]

Using kasprintf to get the function name makes us look up the name
twice, along with all the vsnprintf overhead of parsing the format
string etc. It also means there is an allocation failure case to deal
with. Since symbol_string in vsprintf.c would anyway allocate an array
of size KSYM_SYMBOL_LEN on the stack, that might as well be done up
here.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---

Notes:
    I don't know how expensive it is to do the symbol lookup for each
    initcall. It might be worthwhile adding an
    
      if (list_empty(&blacklisted_initcalls))
        return false;
    
    at the very beginning of initcall_blacklisted(), since this is a debug
    feature and the blacklist is indeed usually empty.

 init/main.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/init/main.c b/init/main.c
index 61b993767db5..286602b677de 100644
--- a/init/main.c
+++ b/init/main.c
@@ -733,22 +733,18 @@ static bool __init_or_module initcall_blacklisted(initcall_t fn)
 {
 	struct list_head *tmp;
 	struct blacklist_entry *entry;
-	char *fn_name;
+	char fn_name[KSYM_SYMBOL_LEN];
 
-	fn_name = kasprintf(GFP_KERNEL, "%pf", fn);
-	if (!fn_name)
-		return false;
+	sprint_symbol_no_offset(fn_name, (unsigned long)fn);
 
 	list_for_each(tmp, &blacklisted_initcalls) {
 		entry = list_entry(tmp, struct blacklist_entry, next);
 		if (!strcmp(fn_name, entry->buf)) {
 			pr_debug("initcall %s blacklisted\n", fn_name);
-			kfree(fn_name);
 			return true;
 		}
 	}
 
-	kfree(fn_name);
 	return false;
 }
 #else
-- 
2.1.3

Re: [RFC/PATCH] init/main.c: Simplify initcall_blacklisted()

[Oleg Nesterov]

On 01/17, Rasmus Villemoes wrote:
>
> Using kasprintf to get the function name makes us look up the name
> twice, along with all the vsnprintf overhead of parsing the format
> string etc. It also means there is an allocation failure case to deal
> with. Since symbol_string in vsprintf.c would anyway allocate an array
> of size KSYM_SYMBOL_LEN on the stack, that might as well be done up
> here.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> 
> Notes:
>     I don't know how expensive it is to do the symbol lookup for each
>     initcall. It might be worthwhile adding an
>     
>       if (list_empty(&blacklisted_initcalls))
>         return false;
>     
>     at the very beginning of initcall_blacklisted(), since this is a debug
>     feature and the blacklist is indeed usually empty.

If we want to optimize this... I am wondering if we can change
initcall_blacklist()

	-	entry->buf = alloc_bootmem(strlen(str_entry) + 1);
	+	ebtry->fn = kallsyms_lookup_name(str_entry);

and then change initcall_blacklisted() to just compare the pointers.

Oleg.

>  init/main.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index 61b993767db5..286602b677de 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -733,22 +733,18 @@ static bool __init_or_module initcall_blacklisted(initcall_t fn)
>  {
>  	struct list_head *tmp;
>  	struct blacklist_entry *entry;
> -	char *fn_name;
> +	char fn_name[KSYM_SYMBOL_LEN];
>  
> -	fn_name = kasprintf(GFP_KERNEL, "%pf", fn);
> -	if (!fn_name)
> -		return false;
> +	sprint_symbol_no_offset(fn_name, (unsigned long)fn);
>  
>  	list_for_each(tmp, &blacklisted_initcalls) {
>  		entry = list_entry(tmp, struct blacklist_entry, next);
>  		if (!strcmp(fn_name, entry->buf)) {
>  			pr_debug("initcall %s blacklisted\n", fn_name);
> -			kfree(fn_name);
>  			return true;
>  		}
>  	}
>  
> -	kfree(fn_name);
>  	return false;
>  }
>  #else
> -- 
> 2.1.3
> 

Re: [RFC/PATCH] init/main.c: Simplify initcall_blacklisted()

[Rusty Russell]

Oleg Nesterov <oleg@redhat.com> writes:
> On 01/17, Rasmus Villemoes wrote:
>>
>> Using kasprintf to get the function name makes us look up the name
>> twice, along with all the vsnprintf overhead of parsing the format
>> string etc. It also means there is an allocation failure case to deal
>> with. Since symbol_string in vsprintf.c would anyway allocate an array
>> of size KSYM_SYMBOL_LEN on the stack, that might as well be done up
>> here.
>> 
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>> 
>> Notes:
>>     I don't know how expensive it is to do the symbol lookup for each
>>     initcall. It might be worthwhile adding an
>>     
>>       if (list_empty(&blacklisted_initcalls))
>>         return false;
>>     
>>     at the very beginning of initcall_blacklisted(), since this is a debug
>>     feature and the blacklist is indeed usually empty.
>
> If we want to optimize this... I am wondering if we can change
> initcall_blacklist()
>
> 	-	entry->buf = alloc_bootmem(strlen(str_entry) + 1);
> 	+	ebtry->fn = kallsyms_lookup_name(str_entry);
>
> and then change initcall_blacklisted() to just compare the pointers.

That would make far, far more sense.  It would fail for modules of
course, but that might be OK.  Prarit, this was your code; does it
matter?

Cheers,
Rusty.

Re: [RFC/PATCH] init/main.c: Simplify initcall_blacklisted()

[Prarit Bhargava]

On 01/19/2015 08:05 PM, Rusty Russell wrote:
> Oleg Nesterov <oleg@redhat.com> writes:
>> On 01/17, Rasmus Villemoes wrote:
>>>
>>> Using kasprintf to get the function name makes us look up the name
>>> twice, along with all the vsnprintf overhead of parsing the format
>>> string etc. It also means there is an allocation failure case to deal
>>> with. Since symbol_string in vsprintf.c would anyway allocate an array
>>> of size KSYM_SYMBOL_LEN on the stack, that might as well be done up
>>> here.
>>>
>>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>>> ---
>>>
>>> Notes:
>>>     I don't know how expensive it is to do the symbol lookup for each
>>>     initcall. It might be worthwhile adding an
>>>     
>>>       if (list_empty(&blacklisted_initcalls))
>>>         return false;
>>>     
>>>     at the very beginning of initcall_blacklisted(), since this is a debug
>>>     feature and the blacklist is indeed usually empty.
>>
>> If we want to optimize this... I am wondering if we can change
>> initcall_blacklist()
>>
>> 	-	entry->buf = alloc_bootmem(strlen(str_entry) + 1);
>> 	+	ebtry->fn = kallsyms_lookup_name(str_entry);
>>
>> and then change initcall_blacklisted() to just compare the pointers.
> 
> That would make far, far more sense.  It would fail for modules of
> course, but that might be OK.  Prarit, this was your code; does it
> matter?

It does actually matter to me.  I've been using it to blacklist modules at boot
as well ... and it works really well :)  So I'm okay with the original patch but
not the second suggested change.

P.

> 
> Cheers,
> Rusty.
> 

Re: [RFC/PATCH] init/main.c: Simplify initcall_blacklisted()

[Oleg Nesterov]

On 01/20, Prarit Bhargava wrote:
>
> On 01/19/2015 08:05 PM, Rusty Russell wrote:
> > Oleg Nesterov <oleg@redhat.com> writes:
> >>
> >> If we want to optimize this... I am wondering if we can change
> >> initcall_blacklist()
> >>
> >> 	-	entry->buf = alloc_bootmem(strlen(str_entry) + 1);
> >> 	+	ebtry->fn = kallsyms_lookup_name(str_entry);
> >>
> >> and then change initcall_blacklisted() to just compare the pointers.
> >
> > That would make far, far more sense.  It would fail for modules of
> > course, but that might be OK.  Prarit, this was your code; does it
> > matter?
>
> It does actually matter to me.  I've been using it to blacklist modules at boot
> as well ... and it works really well :)  So I'm okay with the original patch but
> not the second suggested change.

Yes, I didn't know/realize that initcall_blacklist paramater can be
also used to disable the modules, thanks for correcting me.

But I'd say that initcall_blacklisted(mod->init) looks a bit strange,
I mean it would be probably better to use mod->name in this case, not
the "internal" name of this likely static function.

Perhaps even another kernel parameter makes sense for this, I dunno..
>From Documentation/kernel-parameters.txt:

	initcall_blacklist=  [KNL] Do not execute a comma-separated list of
			initcall functions.  Useful for debugging built-in
			modules and initcalls.

note that this only mentions built-in modules.

Nevermind, I was wrong anyway. Thanks!

Oleg.

Re: [RFC/PATCH] init/main.c: Simplify initcall_blacklisted()

[Prarit Bhargava]

On 01/20/2015 01:05 PM, Oleg Nesterov wrote:
> On 01/20, Prarit Bhargava wrote:
>>
>> On 01/19/2015 08:05 PM, Rusty Russell wrote:
>>> Oleg Nesterov <oleg@redhat.com> writes:
>>>>
>>>> If we want to optimize this... I am wondering if we can change
>>>> initcall_blacklist()
>>>>
>>>> 	-	entry->buf = alloc_bootmem(strlen(str_entry) + 1);
>>>> 	+	ebtry->fn = kallsyms_lookup_name(str_entry);
>>>>
>>>> and then change initcall_blacklisted() to just compare the pointers.
>>>
>>> That would make far, far more sense.  It would fail for modules of
>>> course, but that might be OK.  Prarit, this was your code; does it
>>> matter?
>>
>> It does actually matter to me.  I've been using it to blacklist modules at boot
>> as well ... and it works really well :)  So I'm okay with the original patch but
>> not the second suggested change.
> 
> Yes, I didn't know/realize that initcall_blacklist paramater can be
> also used to disable the modules, thanks for correcting me.

I didn't have that in mind originally, but I've been using it to debug initramfs
module loading.  It has worked quite well.

> 
> But I'd say that initcall_blacklisted(mod->init) looks a bit strange,
> I mean it would be probably better to use mod->name in this case, not
> the "internal" name of this likely static function.

:)  I've been thinking about exactly this too.  I just haven't had any time to
do it.

> 
> Perhaps even another kernel parameter makes sense for this, I dunno..
> From Documentation/kernel-parameters.txt:
> 
> 	initcall_blacklist=  [KNL] Do not execute a comma-separated list of
> 			initcall functions.  Useful for debugging built-in
> 			modules and initcalls.
> 
> note that this only mentions built-in modules.

I can fix that up too.

P.

> 
> Nevermind, I was wrong anyway. Thanks!
> 
> Oleg.
> 

[PATCH resend] init/main.c: Simplify initcall_blacklisted()

[Rasmus Villemoes]

Using kasprintf to get the function name makes us look up the name
twice, along with all the vsnprintf overhead of parsing the format
string etc. It also means there is an allocation failure case to deal
with. Since symbol_string in vsprintf.c would anyway allocate an array
of size KSYM_SYMBOL_LEN on the stack, that might as well be done up
here.

Moreover, since this is a debug feature and the blacklisted_initcalls
list is usually empty, we might as well test that and thus avoid
looking up the symbol name even once in the common case.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 init/main.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/init/main.c b/init/main.c
index b3c6e363ae18..d76d94cd537c 100644
--- a/init/main.c
+++ b/init/main.c
@@ -706,21 +706,20 @@ static int __init initcall_blacklist(char *str)
 static bool __init_or_module initcall_blacklisted(initcall_t fn)
 {
 	struct blacklist_entry *entry;
-	char *fn_name;
+	char fn_name[KSYM_SYMBOL_LEN];
 
-	fn_name = kasprintf(GFP_KERNEL, "%pf", fn);
-	if (!fn_name)
+	if (list_empty(&blacklisted_initcalls))
 		return false;
 
+	sprint_symbol_no_offset(fn_name, (unsigned long)fn);
+
 	list_for_each_entry(entry, &blacklisted_initcalls, next) {
 		if (!strcmp(fn_name, entry->buf)) {
 			pr_debug("initcall %s blacklisted\n", fn_name);
-			kfree(fn_name);
 			return true;
 		}
 	}
 
-	kfree(fn_name);
 	return false;
 }
 #else
-- 
2.1.4

Re: [PATCH resend] init/main.c: Simplify initcall_blacklisted()

[Rusty Russell]

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:
> Using kasprintf to get the function name makes us look up the name
> twice, along with all the vsnprintf overhead of parsing the format
> string etc. It also means there is an allocation failure case to deal
> with. Since symbol_string in vsprintf.c would anyway allocate an array
> of size KSYM_SYMBOL_LEN on the stack, that might as well be done up
> here.
>
> Moreover, since this is a debug feature and the blacklisted_initcalls
> list is usually empty, we might as well test that and thus avoid
> looking up the symbol name even once in the common case.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks!
Rusty.

> ---
>  init/main.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/init/main.c b/init/main.c
> index b3c6e363ae18..d76d94cd537c 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -706,21 +706,20 @@ static int __init initcall_blacklist(char *str)
>  static bool __init_or_module initcall_blacklisted(initcall_t fn)
>  {
>  	struct blacklist_entry *entry;
> -	char *fn_name;
> +	char fn_name[KSYM_SYMBOL_LEN];
>  
> -	fn_name = kasprintf(GFP_KERNEL, "%pf", fn);
> -	if (!fn_name)
> +	if (list_empty(&blacklisted_initcalls))
>  		return false;
>  
> +	sprint_symbol_no_offset(fn_name, (unsigned long)fn);
> +
>  	list_for_each_entry(entry, &blacklisted_initcalls, next) {
>  		if (!strcmp(fn_name, entry->buf)) {
>  			pr_debug("initcall %s blacklisted\n", fn_name);
> -			kfree(fn_name);
>  			return true;
>  		}
>  	}
>  
> -	kfree(fn_name);
>  	return false;
>  }
>  #else
> -- 
> 2.1.4

Re: [PATCH resend] init/main.c: Simplify initcall_blacklisted()

[Rasmus Villemoes]

On Tue, Mar 22 2016, Rusty Russell <rusty@rustcorp.com.au> wrote:

> Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:
>> Using kasprintf to get the function name makes us look up the name
>> twice, along with all the vsnprintf overhead of parsing the format
>> string etc. It also means there is an allocation failure case to deal
>> with. Since symbol_string in vsprintf.c would anyway allocate an array
>> of size KSYM_SYMBOL_LEN on the stack, that might as well be done up
>> here.
>>
>> Moreover, since this is a debug feature and the blacklisted_initcalls
>> list is usually empty, we might as well test that and thus avoid
>> looking up the symbol name even once in the common case.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>
>

Thanks. Andrew, can I get you to take it?

Re: [PATCH resend] init/main.c: Simplify initcall_blacklisted()

[Prarit Bhargava]

On 03/21/2016 07:14 PM, Rasmus Villemoes wrote:
> Using kasprintf to get the function name makes us look up the name
> twice, along with all the vsnprintf overhead of parsing the format
> string etc. It also means there is an allocation failure case to deal
> with. Since symbol_string in vsprintf.c would anyway allocate an array
> of size KSYM_SYMBOL_LEN on the stack, that might as well be done up
> here.
> 
> Moreover, since this is a debug feature and the blacklisted_initcalls
> list is usually empty, we might as well test that and thus avoid
> looking up the symbol name even once in the common case.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>


Acked-by: Prarit Bhargava <prarit@redhat.com>

P.


> ---
>  init/main.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index b3c6e363ae18..d76d94cd537c 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -706,21 +706,20 @@ static int __init initcall_blacklist(char *str)
>  static bool __init_or_module initcall_blacklisted(initcall_t fn)
>  {
>  	struct blacklist_entry *entry;
> -	char *fn_name;
> +	char fn_name[KSYM_SYMBOL_LEN];
>  
> -	fn_name = kasprintf(GFP_KERNEL, "%pf", fn);
> -	if (!fn_name)
> +	if (list_empty(&blacklisted_initcalls))
>  		return false;
>  
> +	sprint_symbol_no_offset(fn_name, (unsigned long)fn);
> +
>  	list_for_each_entry(entry, &blacklisted_initcalls, next) {
>  		if (!strcmp(fn_name, entry->buf)) {
>  			pr_debug("initcall %s blacklisted\n", fn_name);
> -			kfree(fn_name);
>  			return true;
>  		}
>  	}
>  
> -	kfree(fn_name);
>  	return false;
>  }
>  #else
>