[PATCH] kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[PATCH] kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 6 Oct 2017 16:27:26 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 kernel/module.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..07ef44767245 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -837,10 +837,8 @@ static int add_module_usage(struct module *a, struct module *b)
 
 	pr_debug("Allocating new usage for %s.\n", a->name);
 	use = kmalloc(sizeof(*use), GFP_ATOMIC);
-	if (!use) {
-		pr_warn("%s: out of memory loading\n", a->name);
+	if (!use)
 		return -ENOMEM;
-	}
 
 	use->source = a;
 	use->target = b;
-- 
2.14.2

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[Jessica Yu]

+++ SF Markus Elfring [06/10/17 17:12 +0200]:
>From: Markus Elfring <elfring@users.sourceforge.net>
>Date: Fri, 6 Oct 2017 16:27:26 +0200
>
>Omit an extra message for a memory allocation failure in this function.
>
>This issue was detected by using the Coccinelle software.
>
>Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>---
> kernel/module.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index de66ec825992..07ef44767245 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -837,10 +837,8 @@ static int add_module_usage(struct module *a, struct module *b)
>
> 	pr_debug("Allocating new usage for %s.\n", a->name);
> 	use = kmalloc(sizeof(*use), GFP_ATOMIC);
>-	if (!use) {
>-		pr_warn("%s: out of memory loading\n", a->name);
>+	if (!use)
> 		return -ENOMEM;
>-	}

IMO this is removing useful information. Although stack traces are
generated on alloc failures, the extra print also tells us which
module we were trying to load at the time the memory allocation
failed.

Jessica

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[Dan Carpenter]

On Thu, Oct 19, 2017 at 11:29:43AM +0200, Jessica Yu wrote:
> +++ SF Markus Elfring [06/10/17 17:12 +0200]:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Fri, 6 Oct 2017 16:27:26 +0200
> > 
> > Omit an extra message for a memory allocation failure in this function.
> > 
> > This issue was detected by using the Coccinelle software.
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> > kernel/module.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/kernel/module.c b/kernel/module.c
> > index de66ec825992..07ef44767245 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -837,10 +837,8 @@ static int add_module_usage(struct module *a, struct module *b)
> > 
> > 	pr_debug("Allocating new usage for %s.\n", a->name);
> > 	use = kmalloc(sizeof(*use), GFP_ATOMIC);
> > -	if (!use) {
> > -		pr_warn("%s: out of memory loading\n", a->name);
> > +	if (!use)
> > 		return -ENOMEM;
> > -	}
> 
> IMO this is removing useful information. Although stack traces are
> generated on alloc failures, the extra print also tells us which
> module we were trying to load at the time the memory allocation
> failed.

This is a small allocation so it can't fail in current kernels.  I can't
imagine a situation where this could fail and it wasn't dead easy to
debug.  Most modules are loaded at boot so it's not likely to fail, but
if it did, it would be easy to reproduce.  If it's not loaded at boot
it's probably really easy to tell which module we're loading.

regards,
dan carpenter

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[Jessica Yu]

+++ Dan Carpenter [19/10/17 13:30 +0300]:
>On Thu, Oct 19, 2017 at 11:29:43AM +0200, Jessica Yu wrote:
>> +++ SF Markus Elfring [06/10/17 17:12 +0200]:
>> > From: Markus Elfring <elfring@users.sourceforge.net>
>> > Date: Fri, 6 Oct 2017 16:27:26 +0200
>> >
>> > Omit an extra message for a memory allocation failure in this function.
>> >
>> > This issue was detected by using the Coccinelle software.
>> >
>> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> > ---
>> > kernel/module.c | 4 +---
>> > 1 file changed, 1 insertion(+), 3 deletions(-)
>> >
>> > diff --git a/kernel/module.c b/kernel/module.c
>> > index de66ec825992..07ef44767245 100644
>> > --- a/kernel/module.c
>> > +++ b/kernel/module.c
>> > @@ -837,10 +837,8 @@ static int add_module_usage(struct module *a, struct module *b)
>> >
>> > 	pr_debug("Allocating new usage for %s.\n", a->name);
>> > 	use = kmalloc(sizeof(*use), GFP_ATOMIC);
>> > -	if (!use) {
>> > -		pr_warn("%s: out of memory loading\n", a->name);
>> > +	if (!use)
>> > 		return -ENOMEM;
>> > -	}
>>
>> IMO this is removing useful information. Although stack traces are
>> generated on alloc failures, the extra print also tells us which
>> module we were trying to load at the time the memory allocation
>> failed.
>
>This is a small allocation so it can't fail in current kernels.  I can't
>imagine a situation where this could fail and it wasn't dead easy to
>debug.  Most modules are loaded at boot so it's not likely to fail, but
>if it did, it would be easy to reproduce.  If it's not loaded at boot
>it's probably really easy to tell which module we're loading.

Yeah, good points. And on second thought, we normally don't print
warnings for every small alloc failure in the kernel anyway (that
would be utterly superfluous), the error code itself is sufficient.
And in the module loader this seems to be the only printk out of the
dozen alloc calls we do, so I'm OK with removing this one.

Thanks,

Jessica

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[SF Markus Elfring]

>> This is a small allocation so it can't fail in current kernels.  I can't
>> imagine a situation where this could fail and it wasn't dead easy to
>> debug.  Most modules are loaded at boot so it's not likely to fail, but
>> if it did, it would be easy to reproduce.  If it's not loaded at boot
>> it's probably really easy to tell which module we're loading.
> 
> Yeah, good points. And on second thought, we normally don't print
> warnings for every small alloc failure in the kernel anyway (that
> would be utterly superfluous), the error code itself is sufficient.
> And in the module loader this seems to be the only printk out of the
> dozen alloc calls we do, so I'm OK with removing this one.

Thanks for your constructive feedback.

Can it help to improve the corresponding documentation for Linux
programming interfaces a bit more?

Regards,
Markus

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[SF Markus Elfring]

>> @@ -837,10 +837,8 @@ static int add_module_usage(struct module *a, struct module *b)
>>
>>     pr_debug("Allocating new usage for %s.\n", a->name);
>>     use = kmalloc(sizeof(*use), GFP_ATOMIC);
>> -    if (!use) {
>> -        pr_warn("%s: out of memory loading\n", a->name);
>> +    if (!use)
>>         return -ENOMEM;
>> -    }
> 
> IMO this is removing useful information.

How do you think about to clarify the circumstances any further?


> Although stack traces are generated on alloc failures,

Do you ever want to switch them off for special use cases?


> the extra print also tells us which module we were trying to load
> at the time the memory allocation failed.

Can a default allocation failure report provide the same information already?

Regards,
Markus

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[Jessica Yu]

+++ SF Markus Elfring [06/10/17 17:12 +0200]:
>From: Markus Elfring <elfring@users.sourceforge.net>
>Date: Fri, 6 Oct 2017 16:27:26 +0200
>
>Omit an extra message for a memory allocation failure in this function.
>
>This issue was detected by using the Coccinelle software.
>
>Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied to modules-next, thanks.

Jessica

>---
> kernel/module.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index de66ec825992..07ef44767245 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -837,10 +837,8 @@ static int add_module_usage(struct module *a, struct module *b)
>
> 	pr_debug("Allocating new usage for %s.\n", a->name);
> 	use = kmalloc(sizeof(*use), GFP_ATOMIC);
>-	if (!use) {
>-		pr_warn("%s: out of memory loading\n", a->name);
>+	if (!use)
> 		return -ENOMEM;
>-	}
>
> 	use->source = a;
> 	use->target = b;
>-- 
>2.14.2
>

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[SF Markus Elfring]

>> Omit an extra message for a memory allocation failure in this function.
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> 
> Applied to modules-next, thanks.

Thanks for your acceptance of this update suggestion after a bit of clarification.

Do you see any need that I should extend subsequent commit messages
for this software transformation pattern?

Regards,
Markus

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[Joe Perches]

On Thu, 2017-10-19 at 13:18 +0200, SF Markus Elfring wrote:
> > > Omit an extra message for a memory allocation failure in this function.
> > > 
> > > This issue was detected by using the Coccinelle software.
> > > 
> > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > 
> > Applied to modules-next, thanks.
> 
> Thanks for your acceptance of this update suggestion after a bit of clarification.
> 
> Do you see any need that I should extend subsequent commit messages
> for this software transformation pattern?

Add a description of _why_ this is being done.

Something like:

"because there is a dump_stack() done on allocation failures
 without __GFP_JNOWARN"

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[SF Markus Elfring]

>>>> Omit an extra message for a memory allocation failure in this function.
>>>>
>>>> This issue was detected by using the Coccinelle software.
>>>>
>>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>>
>>> Applied to modules-next, thanks.
>>
>> Thanks for your acceptance of this update suggestion after a bit of clarification.
>>
>> Do you see any need that I should extend subsequent commit messages
>> for this software transformation pattern?
> 
> Add a description of _why_ this is being done.
> 
> Something like:
> 
> "because there is a dump_stack() done on allocation failures
>  without __GFP_JNOWARN"

How do you think about to convert such a description into a special format
for further reference documentation?

Regards,
Markus

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[Joe Perches]

On Thu, 2017-10-19 at 13:35 +0200, SF Markus Elfring wrote:
> > > > > Omit an extra message for a memory allocation failure in this function.
> > > > > 
> > > > > This issue was detected by using the Coccinelle software.
[]
> > > Do you see any need that I should extend subsequent commit messages
> > > for this software transformation pattern?
> > 
> > Add a description of _why_ this is being done.
> > 
> > Something like:
> > 
> > "because there is a dump_stack() done on allocation failures
> >  without __GFP_JNOWARN"
> 
> How do you think about to convert such a description into a special format
> for further reference documentation?

I think it's a bad idea if it's a "special" format.

Always write _why_ some code is being changed.

People could read the commit descriptions and would not need
to take extra time to lookup external references.

Maybe add something like
"see (commit <foo> or <file>)" for additional details"

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[SF Markus Elfring]

>>> Something like:
>>>
>>> "because there is a dump_stack() done on allocation failures
>>>  without __GFP_JNOWARN"
>>
>> How do you think about to convert such a description into a special format
>> for further reference documentation?
> 
> I think it's a bad idea if it's a "special" format.

Will it be nice to represent corresponding details as a better
“restructured text”?


> Always write _why_ some code is being changed.
> 
> People could read the commit descriptions and would not need
> to take extra time to lookup external references.

I would appreciate if I could copy a widely accepted explanation.


> Maybe add something like
> "see (commit <foo> or <file>)" for additional details"

Are there any related extensions possible besides other background information?
Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf

Regards,
Markus