[PATCH 0/2] kmemleak: Adjustments for three function implementations

[PATCH 0/2] kmemleak: Adjustments for three function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 14 Aug 2017 11:30:22 +0200

Two update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Delete an error message for a failed memory allocation in two functions
  Use seq_puts() in print_unreferenced()

 mm/kmemleak.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

-- 
2.14.0

[PATCH 1/2] kmemleak: Delete an error message for a failed memory allocation in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 14 Aug 2017 10:50:22 +0200

Omit an extra message for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

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

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 7780cd83a495..c6c798d90b2e 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -555,7 +555,6 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
 
 	object = kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));
 	if (!object) {
-		pr_warn("Cannot allocate a kmemleak_object structure\n");
 		kmemleak_disable();
 		return NULL;
 	}
@@ -775,10 +774,8 @@ static void add_scan_area(unsigned long ptr, size_t size, gfp_t gfp)
 	}
 
 	area = kmem_cache_alloc(scan_area_cache, gfp_kmemleak_mask(gfp));
-	if (!area) {
-		pr_warn("Cannot allocate a scan area\n");
+	if (!area)
 		goto out;
-	}
 
 	spin_lock_irqsave(&object->lock, flags);
 	if (size = SIZE_MAX) {
-- 
2.14.0

Re: [PATCH 1/2] kmemleak: Delete an error message for a failed memory allocation in two functions

[Catalin Marinas]

On Mon, Aug 14, 2017 at 11:35:02AM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 14 Aug 2017 10:50:22 +0200
> 
> Omit an extra message for a memory allocation failure in these functions.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  mm/kmemleak.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 7780cd83a495..c6c798d90b2e 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -555,7 +555,6 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
>  
>  	object = kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));
>  	if (!object) {
> -		pr_warn("Cannot allocate a kmemleak_object structure\n");
>  		kmemleak_disable();

I don't really get what this patch is trying to achieve. Given that
kmemleak will be disabled after this, I'd rather know why it happened.

-- 
Catalin

Re: [PATCH 1/2] kmemleak: Delete an error message for a failed memory allocation in two functions

[SF Markus Elfring]

>> +++ b/mm/kmemleak.c
>> @@ -555,7 +555,6 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
>>  
>>  	object = kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));
>>  	if (!object) {
>> -		pr_warn("Cannot allocate a kmemleak_object structure\n");
>>  		kmemleak_disable();
> 
> I don't really get what this patch is trying to achieve.

I suggest to reduce the code size a bit.


> Given that kmemleak will be disabled after this,

I have got difficulties to interpret this information.


> I'd rather know why it happened.

Do you find the default allocation failure report sufficient?

Regards,
Markus

Re: [PATCH 1/2] kmemleak: Delete an error message for a failed memory allocation in two functions

[Dan Carpenter]

On Mon, Aug 14, 2017 at 12:14:32PM +0100, Catalin Marinas wrote:
> On Mon, Aug 14, 2017 at 11:35:02AM +0200, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Mon, 14 Aug 2017 10:50:22 +0200
> > 
> > Omit an extra message for a memory allocation failure in these functions.
> > 
> > This issue was detected by using the Coccinelle software.
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  mm/kmemleak.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> > index 7780cd83a495..c6c798d90b2e 100644
> > --- a/mm/kmemleak.c
> > +++ b/mm/kmemleak.c
> > @@ -555,7 +555,6 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
> >  
> >  	object = kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));
> >  	if (!object) {
> > -		pr_warn("Cannot allocate a kmemleak_object structure\n");
> >  		kmemleak_disable();
> 
> I don't really get what this patch is trying to achieve. Given that
> kmemleak will be disabled after this, I'd rather know why it happened.

kmem_cache_alloc() will generate a stack trace and a bunch of more
useful information if it fails.  The allocation isn't likely to fail,
but if it does you will know.  The extra message is just wasting RAM.

regards,
dan carpenter

Re: [PATCH 1/2] kmemleak: Delete an error message for a failed memory allocation in two functions

[Catalin Marinas]

On Mon, Aug 14, 2017 at 04:02:21PM +0300, Dan Carpenter wrote:
> On Mon, Aug 14, 2017 at 12:14:32PM +0100, Catalin Marinas wrote:
> > On Mon, Aug 14, 2017 at 11:35:02AM +0200, SF Markus Elfring wrote:
> > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > Date: Mon, 14 Aug 2017 10:50:22 +0200
> > > 
> > > Omit an extra message for a memory allocation failure in these functions.
> > > 
> > > This issue was detected by using the Coccinelle software.
> > > 
> > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > > ---
> > >  mm/kmemleak.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> > > index 7780cd83a495..c6c798d90b2e 100644
> > > --- a/mm/kmemleak.c
> > > +++ b/mm/kmemleak.c
> > > @@ -555,7 +555,6 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
> > >  
> > >  	object = kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));
> > >  	if (!object) {
> > > -		pr_warn("Cannot allocate a kmemleak_object structure\n");
> > >  		kmemleak_disable();
> > 
> > I don't really get what this patch is trying to achieve. Given that
> > kmemleak will be disabled after this, I'd rather know why it happened.
> 
> kmem_cache_alloc() will generate a stack trace and a bunch of more
> useful information if it fails.  The allocation isn't likely to fail,
> but if it does you will know.  The extra message is just wasting RAM.

Currently kmemleak uses __GFP_NOWARN for its own metadata allocation, so
we wouldn't see the sl*b warnings. I don't fully remember why I went for
this gfp flag, probably not to interfere with other messages printed by
the allocator (kmemleak_alloc is called from within sl*b).

I'm fine to drop __GFP_NOWARN and remove those extra messages.

-- 
Catalin

Re: [PATCH 1/2] kmemleak: Delete an error message for a failed memory allocation in two functions

[Dan Carpenter]

On Mon, Aug 14, 2017 at 03:38:04PM +0100, Catalin Marinas wrote:
> On Mon, Aug 14, 2017 at 04:02:21PM +0300, Dan Carpenter wrote:
> > On Mon, Aug 14, 2017 at 12:14:32PM +0100, Catalin Marinas wrote:
> > > On Mon, Aug 14, 2017 at 11:35:02AM +0200, SF Markus Elfring wrote:
> > > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > > Date: Mon, 14 Aug 2017 10:50:22 +0200
> > > > 
> > > > Omit an extra message for a memory allocation failure in these functions.
> > > > 
> > > > This issue was detected by using the Coccinelle software.
> > > > 
> > > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > > > ---
> > > >  mm/kmemleak.c | 5 +----
> > > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > > 
> > > > diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> > > > index 7780cd83a495..c6c798d90b2e 100644
> > > > --- a/mm/kmemleak.c
> > > > +++ b/mm/kmemleak.c
> > > > @@ -555,7 +555,6 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
> > > >  
> > > >  	object = kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));
> > > >  	if (!object) {
> > > > -		pr_warn("Cannot allocate a kmemleak_object structure\n");
> > > >  		kmemleak_disable();
> > > 
> > > I don't really get what this patch is trying to achieve. Given that
> > > kmemleak will be disabled after this, I'd rather know why it happened.
> > 
> > kmem_cache_alloc() will generate a stack trace and a bunch of more
> > useful information if it fails.  The allocation isn't likely to fail,
> > but if it does you will know.  The extra message is just wasting RAM.
> 
> Currently kmemleak uses __GFP_NOWARN for its own metadata allocation, so
> we wouldn't see the sl*b warnings. I don't fully remember why I went for
> this gfp flag, probably not to interfere with other messages printed by
> the allocator (kmemleak_alloc is called from within sl*b).
> 
> I'm fine to drop __GFP_NOWARN and remove those extra messages.
> 

Ah.  I considered that, but didn't see a NOWARN in the diff and was too
lazy to check the code.  Probably I would just leave it as-is.


regards,
dan carpenter

[PATCH 2/2] kmemleak: Use seq_puts() in print_unreferenced()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 14 Aug 2017 11:23:11 +0200

The script "checkpatch.pl" pointed information out like the following.

WARNING: Prefer seq_puts to seq_printf

Thus fix the affected source code place.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 mm/kmemleak.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index c6c798d90b2e..cfac550d4d00 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -373,7 +373,7 @@ static void print_unreferenced(struct seq_file *seq,
 		   object->comm, object->pid, object->jiffies,
 		   msecs_age / 1000, msecs_age % 1000);
 	hex_dump_object(seq, object);
-	seq_printf(seq, "  backtrace:\n");
+	seq_puts(seq, "  backtrace:\n");
 
 	for (i = 0; i < object->trace_len; i++) {
 		void *ptr = (void *)object->trace[i];
-- 
2.14.0