[PATCH 0/2] kmemleak: x86-related patches

[PATCH 0/2] kmemleak: x86-related patches

[Catalin Marinas]

Hi,

I plan to submit these two x86-related patches for 2.6.32. Are the x86
people OK with them (so that I can add the relevant ack)?

Thanks.


Catalin Marinas (2):
      kmemleak: Inform kmemleak about kernel stack allocation
      kmemleak: Ignore the aperture memory hole on x86_64


 Documentation/kmemleak.txt         |   11 +++++------
 arch/x86/include/asm/thread_info.h |    7 ++++++-
 arch/x86/kernel/aperture_64.c      |    6 ++++++
 arch/x86/kernel/pci-dma.c          |    6 ++++++
 arch/x86/kernel/process.c          |    2 ++
 kernel/fork.c                      |    7 ++++++-
 mm/kmemleak.c                      |   22 ----------------------
 7 files changed, 31 insertions(+), 30 deletions(-)

-- 
Catalin

[PATCH 1/2] kmemleak: Inform kmemleak about kernel stack allocation

[Catalin Marinas]

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>
Cc: Ingo Molnar <mingo@redhat.com>
---
 arch/x86/include/asm/thread_info.h |    7 ++++++-
 arch/x86/kernel/process.c          |    2 ++
 kernel/fork.c                      |    7 ++++++-
 3 files changed, 14 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 071166a..a9972db 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -9,6 +9,7 @@
 #include <linux/pm.h>
 #include <linux/clockchips.h>
 #include <linux/random.h>
+#include <linux/kmemleak.h>
 #include <trace/power.h>
 #include <asm/system.h>
 #include <asm/apic.h>
@@ -55,6 +56,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 e6c04d4..ece8199 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

[PATCH 2/2] kmemleak: Ignore the aperture memory hole on x86_64

[Catalin Marinas]

This block is allocated with alloc_bootmem() and scanned by kmemleak but
the kernel direct mapping may no longer exist. This patch tells kmemleak
to ignore this memory hole. The dma32_bootmem_ptr in
dma32_reserve_bootmem() is also ignored.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/aperture_64.c |    6 ++++++
 arch/x86/kernel/pci-dma.c     |    6 ++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
index 676debf..128111d 100644
--- a/arch/x86/kernel/aperture_64.c
+++ b/arch/x86/kernel/aperture_64.c
@@ -20,6 +20,7 @@
 #include <linux/bitops.h>
 #include <linux/ioport.h>
 #include <linux/suspend.h>
+#include <linux/kmemleak.h>
 #include <asm/e820.h>
 #include <asm/io.h>
 #include <asm/iommu.h>
@@ -94,6 +95,11 @@ static u32 __init allocate_aperture(void)
 	 * code for safe
 	 */
 	p = __alloc_bootmem_nopanic(aper_size, aper_size, 512ULL<<20);
+	/*
+	 * Kmemleak should not scan this block as it may not be mapped via the
+	 * kernel direct mapping.
+	 */
+	kmemleak_ignore(p);
 	if (!p || __pa(p)+aper_size > 0xffffffff) {
 		printk(KERN_ERR
 			"Cannot allocate aperture memory hole (%p,%uK)\n",
diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index 1a041bc..fa80f60 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -3,6 +3,7 @@
 #include <linux/dmar.h>
 #include <linux/bootmem.h>
 #include <linux/pci.h>
+#include <linux/kmemleak.h>
 
 #include <asm/proto.h>
 #include <asm/dma.h>
@@ -88,6 +89,11 @@ void __init dma32_reserve_bootmem(void)
 	size = roundup(dma32_bootmem_size, align);
 	dma32_bootmem_ptr = __alloc_bootmem_nopanic(size, align,
 				 512ULL<<20);
+	/*
+	 * Kmemleak should not scan this block as it may not be mapped via the
+	 * kernel direct mapping.
+	 */
+	kmemleak_ignore(dma32_bootmem_ptr);
 	if (dma32_bootmem_ptr)
 		dma32_bootmem_size = size;
 	else

Re: [PATCH 1/2] kmemleak: Inform kmemleak about kernel stack allocation

[Ingo Molnar]

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

> 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>
> Cc: Ingo Molnar <mingo@redhat.com>
> ---
>  arch/x86/include/asm/thread_info.h |    7 ++++++-
>  arch/x86/kernel/process.c          |    2 ++
>  kernel/fork.c                      |    7 ++++++-
>  3 files changed, 14 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;								\
> +})

Sidenote:this used to be a trivial wrapper to gfp so it was 
borderline OK as a CPP macro - now it's a non-trivial CPP wrapper 
macro which is not OK. Mind converting it to an inline function?

	Ingo

Re: [PATCH 2/2] kmemleak: Ignore the aperture memory hole on x86_64

[Ingo Molnar]

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

> This block is allocated with alloc_bootmem() and scanned by kmemleak but
> the kernel direct mapping may no longer exist. This patch tells kmemleak
> to ignore this memory hole. The dma32_bootmem_ptr in
> dma32_reserve_bootmem() is also ignored.
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/aperture_64.c |    6 ++++++
>  arch/x86/kernel/pci-dma.c     |    6 ++++++
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
> index 676debf..128111d 100644
> --- a/arch/x86/kernel/aperture_64.c
> +++ b/arch/x86/kernel/aperture_64.c
> @@ -20,6 +20,7 @@
>  #include <linux/bitops.h>
>  #include <linux/ioport.h>
>  #include <linux/suspend.h>
> +#include <linux/kmemleak.h>
>  #include <asm/e820.h>
>  #include <asm/io.h>
>  #include <asm/iommu.h>
> @@ -94,6 +95,11 @@ static u32 __init allocate_aperture(void)
>  	 * code for safe
>  	 */
>  	p = __alloc_bootmem_nopanic(aper_size, aper_size, 512ULL<<20);
> +	/*
> +	 * Kmemleak should not scan this block as it may not be mapped via the
> +	 * kernel direct mapping.
> +	 */
> +	kmemleak_ignore(p);
>  	if (!p || __pa(p)+aper_size > 0xffffffff) {
>  		printk(KERN_ERR
>  			"Cannot allocate aperture memory hole (%p,%uK)\n",

This sure does not look right for the rare but theoretically 
possible !p case, does it?

> diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> index 1a041bc..fa80f60 100644
> --- a/arch/x86/kernel/pci-dma.c
> +++ b/arch/x86/kernel/pci-dma.c
> @@ -3,6 +3,7 @@
>  #include <linux/dmar.h>
>  #include <linux/bootmem.h>
>  #include <linux/pci.h>
> +#include <linux/kmemleak.h>
>  
>  #include <asm/proto.h>
>  #include <asm/dma.h>
> @@ -88,6 +89,11 @@ void __init dma32_reserve_bootmem(void)
>  	size = roundup(dma32_bootmem_size, align);
>  	dma32_bootmem_ptr = __alloc_bootmem_nopanic(size, align,
>  				 512ULL<<20);
> +	/*
> +	 * Kmemleak should not scan this block as it may not be mapped via the
> +	 * kernel direct mapping.
> +	 */
> +	kmemleak_ignore(dma32_bootmem_ptr);
>  	if (dma32_bootmem_ptr)
>  		dma32_bootmem_size = size;

Same question.

	Ingo

Re: [PATCH 1/2] kmemleak: Inform kmemleak about kernel stack allocation

[Catalin Marinas]

On Sat, 2009-08-29 at 15:29 +0200, Ingo Molnar wrote:
> * Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> > 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>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > ---
> >  arch/x86/include/asm/thread_info.h |    7 ++++++-
> >  arch/x86/kernel/process.c          |    2 ++
> >  kernel/fork.c                      |    7 ++++++-
> >  3 files changed, 14 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;								\
> > +})
> 
> Sidenote:this used to be a trivial wrapper to gfp so it was 
> borderline OK as a CPP macro - now it's a non-trivial CPP wrapper 
> macro which is not OK. Mind converting it to an inline function?

I tried this first but got compilation errors in files that didn't even
call this function. To make it workable, thread_info.h would need to
include additional headers. If that's acceptable, I can post an updated
patch.

Thanks.

-- 
Catalin

Re: [PATCH 2/2] kmemleak: Ignore the aperture memory hole on x86_64

[Catalin Marinas]

On Sat, 2009-08-29 at 15:31 +0200, Ingo Molnar wrote:
> * Catalin Marinas <catalin.marinas@arm.com> wrote:
> > --- a/arch/x86/kernel/aperture_64.c
> > +++ b/arch/x86/kernel/aperture_64.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/bitops.h>
> >  #include <linux/ioport.h>
> >  #include <linux/suspend.h>
> > +#include <linux/kmemleak.h>
> >  #include <asm/e820.h>
> >  #include <asm/io.h>
> >  #include <asm/iommu.h>
> > @@ -94,6 +95,11 @@ static u32 __init allocate_aperture(void)
> >  	 * code for safe
> >  	 */
> >  	p = __alloc_bootmem_nopanic(aper_size, aper_size, 512ULL<<20);
> > +	/*
> > +	 * Kmemleak should not scan this block as it may not be mapped via the
> > +	 * kernel direct mapping.
> > +	 */
> > +	kmemleak_ignore(p);
> >  	if (!p || __pa(p)+aper_size > 0xffffffff) {
> >  		printk(KERN_ERR
> >  			"Cannot allocate aperture memory hole (%p,%uK)\n",
> 
> This sure does not look right for the rare but theoretically 
> possible !p case, does it?

All the kmemleak_* callbacks check for (p && !IS_ERR(p)), just to
simplify the calling site.

-- 
Catalin

Re: [PATCH 1/2] kmemleak: Inform kmemleak about kernel stack allocation

[Catalin Marinas]

On Sat, 2009-08-29 at 15:28 +0100, Catalin Marinas wrote:
> On Sat, 2009-08-29 at 15:29 +0200, Ingo Molnar wrote:
> > * Catalin Marinas <catalin.marinas@arm.com> wrote:
> > 
> > > 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>
> > > Cc: Ingo Molnar <mingo@redhat.com>
> > > ---
> > >  arch/x86/include/asm/thread_info.h |    7 ++++++-
> > >  arch/x86/kernel/process.c          |    2 ++
> > >  kernel/fork.c                      |    7 ++++++-
> > >  3 files changed, 14 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;								\
> > > +})
> > 
> > Sidenote:this used to be a trivial wrapper to gfp so it was 
> > borderline OK as a CPP macro - now it's a non-trivial CPP wrapper 
> > macro which is not OK. Mind converting it to an inline function?
> 
> I tried this first but got compilation errors in files that didn't even
> call this function. To make it workable, thread_info.h would need to
> include additional headers. If that's acceptable, I can post an updated
> patch.

Actually I think it's only the kmemleak.h header but it would get
included via thread_info.h in many other files. An alternative is to
make kmemleak_alloc() etc. return the pointer passed as argument and
keep the macro something like:

kmemleak_alloc(__get_free_pages(...), ...)

though it is still a bit long.

-- 
Catalin

Re: [PATCH 2/2] kmemleak: Ignore the aperture memory hole on x86_64

[Ingo Molnar]

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

> On Sat, 2009-08-29 at 15:31 +0200, Ingo Molnar wrote:
> > * Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > --- a/arch/x86/kernel/aperture_64.c
> > > +++ b/arch/x86/kernel/aperture_64.c
> > > @@ -20,6 +20,7 @@
> > >  #include <linux/bitops.h>
> > >  #include <linux/ioport.h>
> > >  #include <linux/suspend.h>
> > > +#include <linux/kmemleak.h>
> > >  #include <asm/e820.h>
> > >  #include <asm/io.h>
> > >  #include <asm/iommu.h>
> > > @@ -94,6 +95,11 @@ static u32 __init allocate_aperture(void)
> > >  	 * code for safe
> > >  	 */
> > >  	p = __alloc_bootmem_nopanic(aper_size, aper_size, 512ULL<<20);
> > > +	/*
> > > +	 * Kmemleak should not scan this block as it may not be mapped via the
> > > +	 * kernel direct mapping.
> > > +	 */
> > > +	kmemleak_ignore(p);
> > >  	if (!p || __pa(p)+aper_size > 0xffffffff) {
> > >  		printk(KERN_ERR
> > >  			"Cannot allocate aperture memory hole (%p,%uK)\n",
> > 
> > This sure does not look right for the rare but theoretically 
> > possible !p case, does it?
> 
> All the kmemleak_* callbacks check for (p && !IS_ERR(p)), just to
> simplify the calling site.

Indeed, i see. It's OK this way then.

	Ingo

Re: [PATCH 1/2] kmemleak: Inform kmemleak about kernel stack allocation

[Ingo Molnar]

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

> On Sat, 2009-08-29 at 15:29 +0200, Ingo Molnar wrote:
> > * Catalin Marinas <catalin.marinas@arm.com> wrote:
> > 
> > > 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>
> > > Cc: Ingo Molnar <mingo@redhat.com>
> > > ---
> > >  arch/x86/include/asm/thread_info.h |    7 ++++++-
> > >  arch/x86/kernel/process.c          |    2 ++
> > >  kernel/fork.c                      |    7 ++++++-
> > >  3 files changed, 14 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;								\
> > > +})
> > 
> > Sidenote:this used to be a trivial wrapper to gfp so it was 
> > borderline OK as a CPP macro - now it's a non-trivial CPP wrapper 
> > macro which is not OK. Mind converting it to an inline function?
> 
> I tried this first but got compilation errors in files that didn't 
> even call this function. To make it workable, thread_info.h would 
> need to include additional headers. If that's acceptable, I can 
> post an updated patch.

I havent tried the patch myself, but by your description those build 
problems seem to be pre-existing include file dependency problems 
that should be tracked down and resolved - instead of widening them 
by adding even more hidden dependencies via CPP macros.

	Ingo

Re: [PATCH 1/2] kmemleak: Inform kmemleak about kernel stack allocation

[Paul Mundt]

On Thu, Aug 27, 2009 at 06:02:53PM +0100, Catalin Marinas wrote:
> 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>
> Cc: Ingo Molnar <mingo@redhat.com>
> ---
>  arch/x86/include/asm/thread_info.h |    7 ++++++-
>  arch/x86/kernel/process.c          |    2 ++
>  kernel/fork.c                      |    7 ++++++-
>  3 files changed, 14 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;								\
> +})
>
This looks like something that every __HAVE_ARCH_THREAD_INFO_ALLOCATOR
user is going to want to implement. Does it make sense to make this
addition prior to your removal of explicit scanning, or should folks hold
off on this until later in 2.6.32? In any event, this would probably be
worthwhile Cc'ing linux-arch on if you are posting an updated version,
given that there are 9 other architectures that will probably want to do
this, too. 

Re: [PATCH 1/2] kmemleak: Inform kmemleak about kernel stack allocation

[Catalin Marinas]

On Mon, 2009-08-31 at 10:45 +0200, Ingo Molnar wrote:
> * Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> > On Sat, 2009-08-29 at 15:29 +0200, Ingo Molnar wrote:
> > > * Catalin Marinas <catalin.marinas@arm.com> wrote:
[...]
> > > > 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;								\
> > > > +})
> > > 
> > > Sidenote:this used to be a trivial wrapper to gfp so it was 
> > > borderline OK as a CPP macro - now it's a non-trivial CPP wrapper 
> > > macro which is not OK. Mind converting it to an inline function?
> > 
> > I tried this first but got compilation errors in files that didn't 
> > even call this function. To make it workable, thread_info.h would 
> > need to include additional headers. If that's acceptable, I can 
> > post an updated patch.
> 
> I havent tried the patch myself, but by your description those build 
> problems seem to be pre-existing include file dependency problems 
> that should be tracked down and resolved - instead of widening them 
> by adding even more hidden dependencies via CPP macros.

I tried to move to an inline function and linux/gfp.h is needed for
__get_free_pages() and GFP_* macros. This leads to some complicated
circular dependencies like below:

  CC      arch/x86/kernel/asm-offsets.s
In file included from /work/Linux/2.6/linux-2.6-arm/include/linux/mmzone.h:9,
                 from /work/Linux/2.6/linux-2.6-arm/include/linux/gfp.h:4,
                 from /work/Linux/2.6/linux-2.6-arm/arch/x86/include/asm/thread_info.h:22,
                 from /work/Linux/2.6/linux-2.6-arm/include/linux/thread_info.h:56,
                 from /work/Linux/2.6/linux-2.6-arm/include/linux/preempt.h:9,
                 from /work/Linux/2.6/linux-2.6-arm/include/linux/spinlock.h:50,
                 from /work/Linux/2.6/linux-2.6-arm/include/linux/seqlock.h:29,
                 from /work/Linux/2.6/linux-2.6-arm/include/linux/time.h:8,
                 from /work/Linux/2.6/linux-2.6-arm/include/linux/stat.h:60,
                 from /work/Linux/2.6/linux-2.6-arm/include/linux/module.h:10,
                 from /work/Linux/2.6/linux-2.6-arm/include/linux/crypto.h:21,
                 from /work/Linux/2.6/linux-2.6-arm/arch/x86/kernel/asm-offsets_32.c:7,
                 from /work/Linux/2.6/linux-2.6-arm/arch/x86/kernel/asm-offsets.c:2:
include/linux/wait.h|51| error: expected specifier-qualifier-list before ‘spinlock_t’

The linux/mmzone.h file normally includes linux/spinlock.h but the
reverse is also true when the latter includes the former via
thread_info.h and gfp.h. Because of the (correct) guards in the header
files, the spinlock_t definition is no longer available in mmzone.h.


Anyway, what I'd like with this patch is to reduce the latency caused by
holding the tasklist_lock while scanning the stacks and also allow
rescheduling. If you have 500 threads in a system with 8K stacks,
kmemleak would need to scan about 4MB of stacks. While I think I can use
rcu_read_lock/unlock around do_each_thread..while_each_thread,
scheduling still isn't possible.

An alternative would be to traverse the tasks list for every stack
scanned. This traversing should be negligible compared to a stack
scanning time (looking up pointers in the prio tree).

Any opinion/suggestion here?

Thanks.

-- 
Catalin

Re: [PATCH 1/2] kmemleak: Inform kmemleak about kernel stack allocation

[Ingo Molnar]

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

> On Mon, 2009-08-31 at 10:45 +0200, Ingo Molnar wrote:
> > * Catalin Marinas <catalin.marinas@arm.com> wrote:
> > 
> > > On Sat, 2009-08-29 at 15:29 +0200, Ingo Molnar wrote:
> > > > * Catalin Marinas <catalin.marinas@arm.com> wrote:
> [...]
> > > > > 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;								\
> > > > > +})
> > > > 
> > > > Sidenote:this used to be a trivial wrapper to gfp so it was 
> > > > borderline OK as a CPP macro - now it's a non-trivial CPP wrapper 
> > > > macro which is not OK. Mind converting it to an inline function?
> > > 
> > > I tried this first but got compilation errors in files that didn't 
> > > even call this function. To make it workable, thread_info.h would 
> > > need to include additional headers. If that's acceptable, I can 
> > > post an updated patch.
> > 
> > I havent tried the patch myself, but by your description those build 
> > problems seem to be pre-existing include file dependency problems 
> > that should be tracked down and resolved - instead of widening them 
> > by adding even more hidden dependencies via CPP macros.
> 
> I tried to move to an inline function and linux/gfp.h is needed for
> __get_free_pages() and GFP_* macros. This leads to some complicated
> circular dependencies like below:
> 
>   CC      arch/x86/kernel/asm-offsets.s
> In file included from /work/Linux/2.6/linux-2.6-arm/include/linux/mmzone.h:9,
>                  from /work/Linux/2.6/linux-2.6-arm/include/linux/gfp.h:4,
>                  from /work/Linux/2.6/linux-2.6-arm/arch/x86/include/asm/thread_info.h:22,
>                  from /work/Linux/2.6/linux-2.6-arm/include/linux/thread_info.h:56,
>                  from /work/Linux/2.6/linux-2.6-arm/include/linux/preempt.h:9,
>                  from /work/Linux/2.6/linux-2.6-arm/include/linux/spinlock.h:50,
>                  from /work/Linux/2.6/linux-2.6-arm/include/linux/seqlock.h:29,
>                  from /work/Linux/2.6/linux-2.6-arm/include/linux/time.h:8,
>                  from /work/Linux/2.6/linux-2.6-arm/include/linux/stat.h:60,
>                  from /work/Linux/2.6/linux-2.6-arm/include/linux/module.h:10,
>                  from /work/Linux/2.6/linux-2.6-arm/include/linux/crypto.h:21,
>                  from /work/Linux/2.6/linux-2.6-arm/arch/x86/kernel/asm-offsets_32.c:7,
>                  from /work/Linux/2.6/linux-2.6-arm/arch/x86/kernel/asm-offsets.c:2:
> include/linux/wait.h|51| error: expected specifier-qualifier-list before ???spinlock_t???
> 
> The linux/mmzone.h file normally includes linux/spinlock.h but the 
> reverse is also true when the latter includes the former via 
> thread_info.h and gfp.h. Because of the (correct) guards in the 
> header files, the spinlock_t definition is no longer available in 
> mmzone.h.

mmzone.h probably should not include spinlock.h (which is heavy) - 
it should include spinlock_types.h (which is lighter). It only needs 
the type definitions and none of the API definitions.

	Ingo

Re: [PATCH 1/2] kmemleak: Inform kmemleak about kernel stack allocation

[Catalin Marinas]

On Fri, 2009-09-04 at 08:57 +0200, Ingo Molnar wrote:
> * Catalin Marinas <catalin.marinas@arm.com> wrote:
> > I tried to move to an inline function and linux/gfp.h is needed for
> > __get_free_pages() and GFP_* macros. This leads to some complicated
> > circular dependencies like below:
> > 
> >   CC      arch/x86/kernel/asm-offsets.s
> > In file included from /work/Linux/2.6/linux-2.6-arm/include/linux/mmzone.h:9,
> >                  from /work/Linux/2.6/linux-2.6-arm/include/linux/gfp.h:4,
> >                  from /work/Linux/2.6/linux-2.6-arm/arch/x86/include/asm/thread_info.h:22,
> >                  from /work/Linux/2.6/linux-2.6-arm/include/linux/thread_info.h:56,
> >                  from /work/Linux/2.6/linux-2.6-arm/include/linux/preempt.h:9,
> >                  from /work/Linux/2.6/linux-2.6-arm/include/linux/spinlock.h:50,
> >                  from /work/Linux/2.6/linux-2.6-arm/include/linux/seqlock.h:29,
> >                  from /work/Linux/2.6/linux-2.6-arm/include/linux/time.h:8,
> >                  from /work/Linux/2.6/linux-2.6-arm/include/linux/stat.h:60,
> >                  from /work/Linux/2.6/linux-2.6-arm/include/linux/module.h:10,
> >                  from /work/Linux/2.6/linux-2.6-arm/include/linux/crypto.h:21,
> >                  from /work/Linux/2.6/linux-2.6-arm/arch/x86/kernel/asm-offsets_32.c:7,
> >                  from /work/Linux/2.6/linux-2.6-arm/arch/x86/kernel/asm-offsets.c:2:
> > include/linux/wait.h|51| error: expected specifier-qualifier-list before ???spinlock_t???
> > 
> > The linux/mmzone.h file normally includes linux/spinlock.h but the 
> > reverse is also true when the latter includes the former via 
> > thread_info.h and gfp.h. Because of the (correct) guards in the 
> > header files, the spinlock_t definition is no longer available in 
> > mmzone.h.
> 
> mmzone.h probably should not include spinlock.h (which is heavy) - 
> it should include spinlock_types.h (which is lighter). It only needs 
> the type definitions and none of the API definitions.

It fixes this particular case but I get plenty of other similar error in
timex.h, jiffies.h, ktime.h, rcutree.h, kmem.h etc.

I'll drop this patch for now and look for alternatives.

-- 
Catalin