[PATCH v2 0/5] kmemleak: few small cleanups and clear command support

[PATCH v2 0/5] kmemleak: few small cleanups and clear command support

[Luis R. Rodriguez]

This is my second revision, now tested. Does the job I was looking for.
I fixed two issues from my previous untested revision, I removed the scan
lock, and also fixed the strncmp() count.

My kmemleak output was too polluted to do on the fly debugging, adding
a clear command lets you test the sections you want when you want with
a cleared fresh list of kmemleak objects.

Still not sure who this goes through so sending this to Linus.

Luis R. Rodriguez (5):
  kmemleak: use bool for true/false questions
  kmemleak: add clear command support
  kmemleak: move common painting code together
  kmemleak: fix sparse warning over overshadowed flags
  kmemleak: fix sparse warning for static declarations

 Documentation/kmemleak.txt |   17 ++++++++++
 mm/kmemleak.c              |   72 ++++++++++++++++++++++++++++++-------------
 2 files changed, 67 insertions(+), 22 deletions(-)

[PATCH v2 1/5] kmemleak: use bool for true/false questions

[Luis R. Rodriguez]

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 mm/kmemleak.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 4872673..c6e71bb 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -264,17 +264,17 @@ static void kmemleak_disable(void);
  * Newly created objects don't have any color assigned (object->count == -1)
  * before the next memory scan when they become white.
  */
-static int color_white(const struct kmemleak_object *object)
+static bool color_white(const struct kmemleak_object *object)
 {
 	return object->count != -1 && object->count < object->min_count;
 }
 
-static int color_gray(const struct kmemleak_object *object)
+static bool color_gray(const struct kmemleak_object *object)
 {
 	return object->min_count != -1 && object->count >= object->min_count;
 }
 
-static int color_black(const struct kmemleak_object *object)
+static bool color_black(const struct kmemleak_object *object)
 {
 	return object->min_count == -1;
 }
@@ -284,7 +284,7 @@ static int color_black(const struct kmemleak_object *object)
  * not be deleted and have a minimum age to avoid false positives caused by
  * pointers temporarily stored in CPU registers.
  */
-static int unreferenced_object(struct kmemleak_object *object)
+static bool unreferenced_object(struct kmemleak_object *object)
 {
 	return (object->flags & OBJECT_ALLOCATED) && color_white(object) &&
 		time_before_eq(object->jiffies + jiffies_min_age,
-- 
1.6.3.3

[PATCH v2 2/5] kmemleak: add clear command support

[Luis R. Rodriguez]

In an ideal world your kmemleak output will be small,
when its not you can use the clear command to ingore previously
annotated kmemleak objects. We do this by painting them black.

You can now use 'clear'; run some critical section code you'd
like to test, and then run 'scan'.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 Documentation/kmemleak.txt |   17 +++++++++++++++++
 mm/kmemleak.c              |   22 ++++++++++++++++++++++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/Documentation/kmemleak.txt b/Documentation/kmemleak.txt
index 8906803..235090b 100644
--- a/Documentation/kmemleak.txt
+++ b/Documentation/kmemleak.txt
@@ -27,6 +27,13 @@ To trigger an intermediate memory scan:
 
   # echo scan > /sys/kernel/debug/kmemleak
 
+To clear the list of all current possible memory leaks:
+
+  # echo clear > /sys/kernel/debug/kmemleak
+
+New leaks will then come up upon reading /sys/kernel/debug/kmemleak
+again.
+
 Note that the orphan objects are listed in the order they were allocated
 and one object at the beginning of the list may cause other subsequent
 objects to be reported as orphan.
@@ -35,6 +42,8 @@ Memory scanning parameters can be modified at run-time by writing to the
 /sys/kernel/debug/kmemleak file. The following parameters are supported:
 
   off		- disable kmemleak (irreversible)
+  clear		- clear list of current memory leak suspects, done by
+		  marking all current kmemleak objects as black.
   stack=on	- enable the task stacks scanning (default)
   stack=off	- disable the tasks stacks scanning
   scan=on	- start the automatic memory scanning thread (default)
@@ -79,6 +88,8 @@ The scanning algorithm steps:
      gray set is finished
   4. the remaining white objects are considered orphan and reported via
      /sys/kernel/debug/kmemleak
+  5. All black objects are ignored on the output of
+     /sys/kernel/debug/kmemleak
 
 Some allocated memory blocks have pointers stored in the kernel's
 internal data structures and they cannot be detected as orphans. To
@@ -86,6 +97,12 @@ avoid this, kmemleak can also store the number of values pointing to an
 address inside the block address range that need to be found so that the
 block is not considered a leak. One example is __vmalloc().
 
+Testing a sections with kmemleak
+--------------------------------
+
+You can use the 'clear' and 'scan' command over certain critical sections you
+would like to test.
+
 Kmemleak API
 ------------
 
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index c6e71bb..b6bc34e 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1294,10 +1294,30 @@ static int kmemleak_release(struct inode *inode, struct file *file)
 	return seq_release(inode, file);
 }
 
+static void kmemleak_clear(void)
+{
+	struct kmemleak_object *object;
+	unsigned long flags;
+
+	stop_scan_thread();
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(object, &object_list, object_list) {
+		spin_lock_irqsave(&object->lock, flags);
+		object->min_count = -1;
+		spin_unlock_irqrestore(&object->lock, flags);
+	}
+	rcu_read_unlock();
+
+	start_scan_thread();
+}
+
 /*
  * File write operation to configure kmemleak at run-time. The following
  * commands can be written to the /sys/kernel/debug/kmemleak file:
  *   off	- disable kmemleak (irreversible)
+ *   clear	- mark all current kmemleak objects as black to ingore
+ *		  printing them
  *   stack=on	- enable the task stacks scanning
  *   stack=off	- disable the tasks stacks scanning
  *   scan=on	- start the automatic memory scanning thread
@@ -1324,6 +1344,8 @@ static ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
 
 	if (strncmp(buf, "off", 3) == 0)
 		kmemleak_disable();
+	else if (strncmp(buf, "clear", 5) == 0)
+		kmemleak_clear();
 	else if (strncmp(buf, "stack=on", 8) == 0)
 		kmemleak_stack_scan = 1;
 	else if (strncmp(buf, "stack=off", 9) == 0)
-- 
1.6.3.3

Re: [PATCH v2 2/5] kmemleak: add clear command support

[Catalin Marinas]

On Thu, 2009-09-03 at 01:35 -0400, Luis R. Rodriguez wrote:
> In an ideal world your kmemleak output will be small,
> when its not you can use the clear command to ingore previously
> annotated kmemleak objects. We do this by painting them black.

Making the objects "black" means that they are completely ignored by
kmemleak and they are assumed not to contain any valid references.
Therefore they won't be scanned and many of the newly allocated objects
would be false positives.

You may want to make them "gray" and only those which were reported as
unreferenced, something like below:

	if ((object->flags & OBJECT_REPORTED) && unreferenced_object(object))
		make_gray_object(object->pointer)

-- 
Catalin

Re: [PATCH v2 2/5] kmemleak: add clear command support

[Luis R. Rodriguez]

On Thu, Sep 03, 2009 at 09:52:11AM -0700, Catalin Marinas wrote:
> On Thu, 2009-09-03 at 01:35 -0400, Luis R. Rodriguez wrote:
> > In an ideal world your kmemleak output will be small,
> > when its not you can use the clear command to ingore previously
> > annotated kmemleak objects. We do this by painting them black.
> 
> Making the objects "black" means that they are completely ignored by
> kmemleak and they are assumed not to contain any valid references.
> Therefore they won't be scanned and many of the newly allocated objects
> would be false positives.

Got it, BTW can you elaborate as to why painting objects black would
create false positives for newly allocated objects? I fail to understand
why.

> You may want to make them "gray" and only those which were reported as
> unreferenced, something like below:
> 
>         if ((object->flags & OBJECT_REPORTED) && unreferenced_object(object))
>                 make_gray_object(object->pointer)

Thanks, will use this.

  Luis

Re: [PATCH v2 2/5] kmemleak: add clear command support

[Catalin Marinas]

On Fri, 2009-09-04 at 13:26 -0700, Luis R. Rodriguez wrote:
> On Thu, Sep 03, 2009 at 09:52:11AM -0700, Catalin Marinas wrote:
> > On Thu, 2009-09-03 at 01:35 -0400, Luis R. Rodriguez wrote:
> > > In an ideal world your kmemleak output will be small,
> > > when its not you can use the clear command to ingore previously
> > > annotated kmemleak objects. We do this by painting them black.
> > 
> > Making the objects "black" means that they are completely ignored by
> > kmemleak and they are assumed not to contain any valid references.
> > Therefore they won't be scanned and many of the newly allocated objects
> > would be false positives.
> 
> Got it, BTW can you elaborate as to why painting objects black would
> create false positives for newly allocated objects? I fail to understand
> why.

Black objects are not scanned by kmemleak. Making valid objects black as
per your patch, they may (actually quite likely) contain pointers to
newly allocated objects. Since they won't be scanned, the newly
allocated objects are reported as unreferenced as no pointers to them
were found during scanning.

-- 
Catalin

[PATCH v2 3/5] kmemleak: move common painting code together

[Luis R. Rodriguez]

When painting grey or black we do the same thing, bring
this together into a helper and identify coloring grey or
black explicitly with defines. This makes this a little
easier to read.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 mm/kmemleak.c |   32 +++++++++++++++++++-------------
 1 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index b6bc34e..58c07b1 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -120,6 +120,9 @@ struct kmemleak_scan_area {
 	size_t length;
 };
 
+#define KMEMLEAK_GREY	0
+#define KMEMLEAK_BLACK	-1
+
 /*
  * Structure holding the metadata for each allocated memory block.
  * Modifications to such objects should be made while holding the
@@ -266,17 +269,19 @@ static void kmemleak_disable(void);
  */
 static bool color_white(const struct kmemleak_object *object)
 {
-	return object->count != -1 && object->count < object->min_count;
+	return object->count != KMEMLEAK_BLACK &&
+		object->count < object->min_count;
 }
 
 static bool color_gray(const struct kmemleak_object *object)
 {
-	return object->min_count != -1 && object->count >= object->min_count;
+	return object->min_count != KMEMLEAK_BLACK &&
+		object->count >= object->min_count;
 }
 
 static bool color_black(const struct kmemleak_object *object)
 {
-	return object->min_count == -1;
+	return object->min_count == KMEMLEAK_BLACK;
 }
 
 /*
@@ -604,13 +609,21 @@ static void delete_object_part(unsigned long ptr, size_t size)
 
 	put_object(object);
 }
+
+static void paint_it(struct kmemleak_object *object, int color)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&object->lock, flags);
+	object->min_count = color;
+	spin_unlock_irqrestore(&object->lock, flags);
+}
+
 /*
  * Make a object permanently as gray-colored so that it can no longer be
  * reported as a leak. This is used in general to mark a false positive.
  */
 static void make_gray_object(unsigned long ptr)
 {
-	unsigned long flags;
 	struct kmemleak_object *object;
 
 	object = find_and_get_object(ptr, 0);
@@ -618,10 +631,7 @@ static void make_gray_object(unsigned long ptr)
 		kmemleak_warn("Graying unknown object at 0x%08lx\n", ptr);
 		return;
 	}
-
-	spin_lock_irqsave(&object->lock, flags);
-	object->min_count = 0;
-	spin_unlock_irqrestore(&object->lock, flags);
+	paint_it(object, KMEMLEAK_GREY);
 	put_object(object);
 }
 
@@ -631,7 +641,6 @@ static void make_gray_object(unsigned long ptr)
  */
 static void make_black_object(unsigned long ptr)
 {
-	unsigned long flags;
 	struct kmemleak_object *object;
 
 	object = find_and_get_object(ptr, 0);
@@ -639,10 +648,7 @@ static void make_black_object(unsigned long ptr)
 		kmemleak_warn("Blacking unknown object at 0x%08lx\n", ptr);
 		return;
 	}
-
-	spin_lock_irqsave(&object->lock, flags);
-	object->min_count = -1;
-	spin_unlock_irqrestore(&object->lock, flags);
+	paint_it(object, KMEMLEAK_BLACK);
 	put_object(object);
 }
 
-- 
1.6.3.3

[PATCH v2 4/5] kmemleak: fix sparse warning over overshadowed flags

[Luis R. Rodriguez]

This fixes this sparse warning:
mm/kmemleak.c:512:31: warning: symbol 'flags' shadows an earlier one
mm/kmemleak.c:448:23: originally declared here

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 mm/kmemleak.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 58c07b1..24e7a84 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -509,14 +509,14 @@ static void create_object(unsigned long ptr, size_t size, int min_count,
 	 * random memory blocks.
 	 */
 	if (node != &object->tree_node) {
-		unsigned long flags;
+		unsigned long flags_object;
 
 		kmemleak_stop("Cannot insert 0x%lx into the object search tree "
 			      "(already existing)\n", ptr);
 		object = lookup_object(ptr, 1);
-		spin_lock_irqsave(&object->lock, flags);
+		spin_lock_irqsave(&object->lock, flags_object);
 		dump_object_info(object);
-		spin_unlock_irqrestore(&object->lock, flags);
+		spin_unlock_irqrestore(&object->lock, flags_object);
 
 		goto out;
 	}
-- 
1.6.3.3

Re: [PATCH v2 4/5] kmemleak: fix sparse warning over overshadowed flags

[Catalin Marinas]

On Thu, 2009-09-03 at 01:35 -0400, Luis R. Rodriguez wrote:
> This fixes this sparse warning:
> mm/kmemleak.c:512:31: warning: symbol 'flags' shadows an earlier one
> mm/kmemleak.c:448:23: originally declared here
> 
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
>  mm/kmemleak.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 58c07b1..24e7a84 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -509,14 +509,14 @@ static void create_object(unsigned long ptr, size_t size, int min_count,
>  	 * random memory blocks.
>  	 */
>  	if (node != &object->tree_node) {
> -		unsigned long flags;
> +		unsigned long flags_object;
>  
>  		kmemleak_stop("Cannot insert 0x%lx into the object search tree "
>  			      "(already existing)\n", ptr);
>  		object = lookup_object(ptr, 1);
> -		spin_lock_irqsave(&object->lock, flags);
> +		spin_lock_irqsave(&object->lock, flags_object);
>  		dump_object_info(object);
> -		spin_unlock_irqrestore(&object->lock, flags);
> +		spin_unlock_irqrestore(&object->lock, flags_object);

As Pekka said, we only need spin_lock() variant here as the interrupts
are already disabled.

-- 
Catalin

Re: [PATCH v2 4/5] kmemleak: fix sparse warning over overshadowed flags

[Luis R. Rodriguez]

On Thu, Sep 3, 2009 at 9:55 AM, Catalin Marinas<catalin.marinas@arm.com> wrote:
> On Thu, 2009-09-03 at 01:35 -0400, Luis R. Rodriguez wrote:
>> This fixes this sparse warning:
>> mm/kmemleak.c:512:31: warning: symbol 'flags' shadows an earlier one
>> mm/kmemleak.c:448:23: originally declared here
>>
>> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
>> ---
>>  mm/kmemleak.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>> index 58c07b1..24e7a84 100644
>> --- a/mm/kmemleak.c
>> +++ b/mm/kmemleak.c
>> @@ -509,14 +509,14 @@ static void create_object(unsigned long ptr, size_t size, int min_count,
>>        * random memory blocks.
>>        */
>>       if (node != &object->tree_node) {
>> -             unsigned long flags;
>> +             unsigned long flags_object;
>>
>>               kmemleak_stop("Cannot insert 0x%lx into the object search tree "
>>                             "(already existing)\n", ptr);
>>               object = lookup_object(ptr, 1);
>> -             spin_lock_irqsave(&object->lock, flags);
>> +             spin_lock_irqsave(&object->lock, flags_object);
>>               dump_object_info(object);
>> -             spin_unlock_irqrestore(&object->lock, flags);
>> +             spin_unlock_irqrestore(&object->lock, flags_object);
>
> As Pekka said, we only need spin_lock() variant here as the interrupts
> are already disabled.

OK will respin thanks.

  Luis

[PATCH v2 5/5] kmemleak: fix sparse warning for static declarations

[Luis R. Rodriguez]

This fixes these sparse warnings:

mm/kmemleak.c:1179:6: warning: symbol 'start_scan_thread' was not declared. Should it be static?
mm/kmemleak.c:1194:6: warning: symbol 'stop_scan_thread' was not declared. Should it be static?

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 mm/kmemleak.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 24e7a84..262e155 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1176,7 +1176,7 @@ static int kmemleak_scan_thread(void *arg)
  * Start the automatic memory scanning thread. This function must be called
  * with the scan_mutex held.
  */
-void start_scan_thread(void)
+static void start_scan_thread(void)
 {
 	if (scan_thread)
 		return;
@@ -1191,7 +1191,7 @@ void start_scan_thread(void)
  * Stop the automatic memory scanning thread. This function must be called
  * with the scan_mutex held.
  */
-void stop_scan_thread(void)
+static void stop_scan_thread(void)
 {
 	if (scan_thread) {
 		kthread_stop(scan_thread);
-- 
1.6.3.3

Re: [PATCH v2 0/5] kmemleak: few small cleanups and clear command support

[Pekka Enberg]

Hi Luis,

On Thu, Sep 3, 2009 at 8:35 AM, Luis R. Rodriguez<lrodriguez@atheros.com> wrote:
> Still not sure who this goes through so sending this to Linus.

Time zone difference alert! I'm sorry but I haven't yet figured out
how to answer my emails while I'm sleeping. :-)

Seriously though, the patches should go through Catalin's git tree
that's hiding somewhere.

                        Pekka

Re: [PATCH v2 0/5] kmemleak: few small cleanups and clear command support

[Catalin Marinas]

On Thu, 2009-09-03 at 01:35 -0400, Luis R. Rodriguez wrote:
> Still not sure who this goes through so sending this to Linus.

As Pekka said, in some parts of the world the working day just started.
BTW, as I'm the kmemleak maintainer, the patches should probably go
through me (or at least get an ack), especially since I have other
kmemleak patches to push (the kmemleak branch on
git://linux-arm.org/linux-2.6.git).

Thanks. I'll have a look at the patches today.

-- 
Catalin

Re: [PATCH v2 0/5] kmemleak: few small cleanups and clear command support

[Luis R. Rodriguez]

On Thu, Sep 03, 2009 at 01:30:13AM -0700, Catalin Marinas wrote:
> On Thu, 2009-09-03 at 01:35 -0400, Luis R. Rodriguez wrote:
> > Still not sure who this goes through so sending this to Linus.
> 
> As Pekka said, in some parts of the world the working day just started.
> BTW, as I'm the kmemleak maintainer, the patches should probably go
> through me (or at least get an ack), especially since I have other
> kmemleak patches to push (the kmemleak branch on
> git://linux-arm.org/linux-2.6.git).

I'll respin on this tree with some more changes requested.

  Luis