[RFC PATCH] kmemleak: Scan all thread stacks

[RFC PATCH] kmemleak: Scan all thread stacks

[Catalin Marinas]

This patch changes the for_each_process() loop with the
do_each_thread()/while_each_thread() pair. It also replaces the
read_lock(&tasklist_lock) with rcu_read_lock() and task_lock(p).

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---

My questions:

1. Is it correct that for_each_process() used currently by kmemleak may
   not loop through all the possible kernel thread stacks?

2. Is it safe to use rcu_read_lock() and task_lock() when scanning the
   corresponding kernel stack (thread_info structure)? The loop doesn't
   do any modification to the task list. The reason for this is to
   allow kernel preemption when scanning the stacks.

Alternatively, I can hook kmemleak callbacks into the
alloc_thread_info/free_thread_info structures but many of these are
architecture-specific.

Thanks,

Catalin


 mm/kmemleak.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 983f3f6..a933128 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1064,7 +1064,6 @@ static void kmemleak_scan(void)
 {
 	unsigned long flags;
 	struct kmemleak_object *object, *tmp;
-	struct task_struct *task;
 	int i;
 	int new_leaks = 0;
 	int gray_list_pass = 0;
@@ -1135,12 +1134,16 @@ static void kmemleak_scan(void)
 	 * not enabled by default.
 	 */
 	if (kmemleak_stack_scan) {
-		read_lock(&tasklist_lock);
-		for_each_process(task)
-			scan_block(task_stack_page(task),
-				   task_stack_page(task) + THREAD_SIZE,
-				   NULL, 0);
-		read_unlock(&tasklist_lock);
+		struct task_struct *p, *g;
+
+		rcu_read_lock();
+		do_each_thread(g, p) {
+			task_lock(p);
+			scan_block(task_stack_page(p), task_stack_page(p) +
+				   THREAD_SIZE, NULL, 0);
+			task_unlock(p);
+		} while_each_thread(g, p);
+		rcu_read_unlock();
 	}
 
 	/*

Re: [RFC PATCH] kmemleak: Scan all thread stacks

[Ingo Molnar]

* Catalin Marinas <catalin.marinas@arm.com> wrote:

> This patch changes the for_each_process() loop with the
> do_each_thread()/while_each_thread() pair. It also replaces the
> read_lock(&tasklist_lock) with rcu_read_lock() and task_lock(p).
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
> 
> My questions:
> 
> 1. Is it correct that for_each_process() used currently by kmemleak may
>    not loop through all the possible kernel thread stacks?

yes. You need the full, all tasks variant:

        do_each_thread(g, p) {
        ...
        } while_each_thread(g, p);


> 2. Is it safe to use rcu_read_lock() and task_lock() when scanning the
>    corresponding kernel stack (thread_info structure)? The loop doesn't
>    do any modification to the task list. The reason for this is to
>    allow kernel preemption when scanning the stacks.

you cannot generally preempt while holding the RCU read-lock.

	Ingo

Re: [RFC PATCH] kmemleak: Scan all thread stacks

[Catalin Marinas]

On Fri, 2009-07-17 at 18:43 +0200, Ingo Molnar wrote:
> * Catalin Marinas <catalin.marinas@arm.com> wrote:
> > 2. Is it safe to use rcu_read_lock() and task_lock() when scanning the
> >    corresponding kernel stack (thread_info structure)? The loop doesn't
> >    do any modification to the task list. The reason for this is to
> >    allow kernel preemption when scanning the stacks.
> 
> you cannot generally preempt while holding the RCU read-lock.

This may work with rcupreempt enabled. But, with classic RCU is it safe
to call schedule (or cond_resched) while holding the RCU read-lock?

Thanks.

-- 
Catalin

Re: [RFC PATCH] kmemleak: Scan all thread stacks

[Peter Zijlstra]

On Fri, 2009-07-17 at 17:57 +0100, Catalin Marinas wrote:
> On Fri, 2009-07-17 at 18:43 +0200, Ingo Molnar wrote:
> > * Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > 2. Is it safe to use rcu_read_lock() and task_lock() when scanning the
> > >    corresponding kernel stack (thread_info structure)? The loop doesn't
> > >    do any modification to the task list. The reason for this is to
> > >    allow kernel preemption when scanning the stacks.
> > 
> > you cannot generally preempt while holding the RCU read-lock.
> 
> This may work with rcupreempt enabled. But, with classic RCU is it safe
> to call schedule (or cond_resched) while holding the RCU read-lock?

No.

Re: [RFC PATCH] kmemleak: Scan all thread stacks

[Catalin Marinas]

On Fri, 2009-07-17 at 19:01 +0200, Peter Zijlstra wrote:
> On Fri, 2009-07-17 at 17:57 +0100, Catalin Marinas wrote:
> > On Fri, 2009-07-17 at 18:43 +0200, Ingo Molnar wrote:
> > > * Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > 2. Is it safe to use rcu_read_lock() and task_lock() when scanning the
> > > >    corresponding kernel stack (thread_info structure)? The loop doesn't
> > > >    do any modification to the task list. The reason for this is to
> > > >    allow kernel preemption when scanning the stacks.
> > > 
> > > you cannot generally preempt while holding the RCU read-lock.
> > 
> > This may work with rcupreempt enabled. But, with classic RCU is it safe
> > to call schedule (or cond_resched) while holding the RCU read-lock?
> 
> No.

Thanks for the clarification. So, to really make a difference in
latency, task stack scanning in kmemleak doesn't need to be explicit.
The patch below would be required:


kmemleak: Inform kmemleak about kernel stack allocation

From: Catalin Marinas <catalin.marinas@arm.com>

Traversing all the tasks in the system for scanning the kernel stacks
requires locking which increases the kernel latency considerably. This
patch informs kmemleak about newly allocated or freed stacks so that
they are treated as any other allocated object. Subsequent patch will
remove the explicit stack scanning from mm/kmemleak.c.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/x86/include/asm/thread_info.h |    7 ++++++-
 arch/x86/kernel/process.c          |    1 +
 kernel/fork.c                      |    7 ++++++-
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index fad7d40..f26432a 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -162,7 +162,12 @@ struct thread_info {
 #define __HAVE_ARCH_THREAD_INFO_ALLOCATOR
 
 #define alloc_thread_info(tsk)						\
-	((struct thread_info *)__get_free_pages(THREAD_FLAGS, THREAD_ORDER))
+({									\
+	struct thread_info *ti = (struct thread_info *)			\
+		__get_free_pages(THREAD_FLAGS, THREAD_ORDER);		\
+	kmemleak_alloc(ti, THREAD_SIZE, 1, THREAD_FLAGS);		\
+	ti;								\
+})
 
 #ifdef CONFIG_X86_32
 
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 994dd6a..ac43992 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -55,6 +55,7 @@ void free_thread_xstate(struct task_struct *tsk)
 void free_thread_info(struct thread_info *ti)
 {
 	free_thread_xstate(ti->task);
+	kmemleak_free(ti);
 	free_pages((unsigned long)ti, get_order(THREAD_SIZE));
 }
 
diff --git a/kernel/fork.c b/kernel/fork.c
index bd29592..31a4e77 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -62,6 +62,7 @@
 #include <linux/fs_struct.h>
 #include <linux/magic.h>
 #include <linux/perf_counter.h>
+#include <linux/kmemleak.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
@@ -104,16 +105,20 @@ static struct kmem_cache *task_struct_cachep;
 #ifndef __HAVE_ARCH_THREAD_INFO_ALLOCATOR
 static inline struct thread_info *alloc_thread_info(struct task_struct *tsk)
 {
+	struct thread_info *ti;
 #ifdef CONFIG_DEBUG_STACK_USAGE
 	gfp_t mask = GFP_KERNEL | __GFP_ZERO;
 #else
 	gfp_t mask = GFP_KERNEL;
 #endif
-	return (struct thread_info *)__get_free_pages(mask, THREAD_SIZE_ORDER);
+	ti = (struct thread_info *)__get_free_pages(mask, THREAD_SIZE_ORDER);
+	kmemleak_alloc(ti, THREAD_SIZE, 1, mask);
+	return ti;
 }
 
 static inline void free_thread_info(struct thread_info *ti)
 {
+	kmemleak_free(ti);
 	free_pages((unsigned long)ti, THREAD_SIZE_ORDER);
 }
 #endif

-- 
Catalin

Re: [RFC PATCH] kmemleak: Scan all thread stacks

[Paul E. McKenney]

On Fri, Jul 17, 2009 at 07:01:09PM +0200, Peter Zijlstra wrote:
> On Fri, 2009-07-17 at 17:57 +0100, Catalin Marinas wrote:
> > On Fri, 2009-07-17 at 18:43 +0200, Ingo Molnar wrote:
> > > * Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > 2. Is it safe to use rcu_read_lock() and task_lock() when scanning the
> > > >    corresponding kernel stack (thread_info structure)? The loop doesn't
> > > >    do any modification to the task list. The reason for this is to
> > > >    allow kernel preemption when scanning the stacks.
> > > 
> > > you cannot generally preempt while holding the RCU read-lock.
> > 
> > This may work with rcupreempt enabled. But, with classic RCU is it safe
> > to call schedule (or cond_resched) while holding the RCU read-lock?
> 
> No.

What Peter said!  ;-)

However, you might be able to use SRCU (http://lwn.net/Articles/202847/),
which does allow blocking within read-side critical sections.

							Thanx, Paul

Re: [RFC PATCH] kmemleak: Scan all thread stacks

[Catalin Marinas]

On Wed, 2009-07-22 at 09:18 -0700, Paul E. McKenney wrote:
> On Fri, Jul 17, 2009 at 07:01:09PM +0200, Peter Zijlstra wrote:
> > On Fri, 2009-07-17 at 17:57 +0100, Catalin Marinas wrote:
> > > On Fri, 2009-07-17 at 18:43 +0200, Ingo Molnar wrote:
> > > > * Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > > 2. Is it safe to use rcu_read_lock() and task_lock() when scanning the
> > > > >    corresponding kernel stack (thread_info structure)? The loop doesn't
> > > > >    do any modification to the task list. The reason for this is to
> > > > >    allow kernel preemption when scanning the stacks.
> > > > 
> > > > you cannot generally preempt while holding the RCU read-lock.
> > > 
> > > This may work with rcupreempt enabled. But, with classic RCU is it safe
> > > to call schedule (or cond_resched) while holding the RCU read-lock?
> > 
> > No.
> 
> What Peter said!  ;-)
> 
> However, you might be able to use SRCU (http://lwn.net/Articles/202847/),
> which does allow blocking within read-side critical sections.

Thanks for the suggestion. But this would mean that the task_struct
creation/deletion code should use the SRCU as well which I wouldn't
modify. I'm also not entirely sure this could replace
read_lock(&tasklist_lock)/read_unlock (as per the initial question).

The simplest fix for kmemleak is to not traverse the task list at all -
http://lkml.org/lkml/2009/7/20/55. The patch is just like any other
kmemleak annotation in the kernel.

Thanks.

-- 
Catalin

Re: [RFC PATCH] kmemleak: Scan all thread stacks

[Paul E. McKenney]

On Wed, Jul 22, 2009 at 06:03:29PM +0100, Catalin Marinas wrote:
> On Wed, 2009-07-22 at 09:18 -0700, Paul E. McKenney wrote:
> > On Fri, Jul 17, 2009 at 07:01:09PM +0200, Peter Zijlstra wrote:
> > > On Fri, 2009-07-17 at 17:57 +0100, Catalin Marinas wrote:
> > > > On Fri, 2009-07-17 at 18:43 +0200, Ingo Molnar wrote:
> > > > > * Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > > > 2. Is it safe to use rcu_read_lock() and task_lock() when scanning the
> > > > > >    corresponding kernel stack (thread_info structure)? The loop doesn't
> > > > > >    do any modification to the task list. The reason for this is to
> > > > > >    allow kernel preemption when scanning the stacks.
> > > > > 
> > > > > you cannot generally preempt while holding the RCU read-lock.
> > > > 
> > > > This may work with rcupreempt enabled. But, with classic RCU is it safe
> > > > to call schedule (or cond_resched) while holding the RCU read-lock?
> > > 
> > > No.
> > 
> > What Peter said!  ;-)
> > 
> > However, you might be able to use SRCU (http://lwn.net/Articles/202847/),
> > which does allow blocking within read-side critical sections.
> 
> Thanks for the suggestion. But this would mean that the task_struct
> creation/deletion code should use the SRCU as well which I wouldn't
> modify. I'm also not entirely sure this could replace
> read_lock(&tasklist_lock)/read_unlock (as per the initial question).
> 
> The simplest fix for kmemleak is to not traverse the task list at all -
> http://lkml.org/lkml/2009/7/20/55. The patch is just like any other
> kmemleak annotation in the kernel.

Even better, agreed!

							Thanx, Paul