[PATCH] kmemleak: Allow kmemleak to be built on powerpc

[PATCH] kmemleak: Allow kmemleak to be built on powerpc

[Michael Ellerman]

Very lightly tested, doesn't crash the kernel.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---

It doesn't look like we actually need to add any support in the
arch code - or is there something I'm missing?


 lib/Kconfig.debug |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 12327b2..d5ca9a5 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -338,7 +338,7 @@ config SLUB_STATS
 
 config DEBUG_KMEMLEAK
 	bool "Kernel memory leak detector"
-	depends on DEBUG_KERNEL && EXPERIMENTAL && (X86 || ARM) && \
+	depends on DEBUG_KERNEL && EXPERIMENTAL && (X86 || ARM || PPC) && \
 		!MEMORY_HOTPLUG
 	select DEBUG_FS if SYSFS
 	select STACKTRACE if STACKTRACE_SUPPORT
-- 
1.6.2.1

Re: [PATCH] kmemleak: Allow kmemleak to be built on powerpc

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1517 bytes --]

On Thu, 2009-07-16 at 11:25 +1000, Michael Ellerman wrote:
> Very lightly tested, doesn't crash the kernel.
> 
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> ---
> 
> It doesn't look like we actually need to add any support in the
> arch code - or is there something I'm missing?

Hmm, I think we want to add annotations in lib/lmb.c don't we? That's
our low-level pre-bootmem allocator.

And we have the same problem with _edata as x86.

And I'm seeing lots (~250) of these:

unreferenced object 0xc0000000fcd2e2f0 (size 16):
  comm "swapper", pid 1, jiffies 4294892393
  backtrace:
    [<c00000000014d9c0>] .create_object+0x164/0x2a8
    [<c00000000014dc94>] .kmemleak_alloc+0x74/0x120
    [<c0000000001474d0>] .__kmalloc+0x20c/0x2ac
    [<c00000000036998c>] .kvasprintf+0x64/0xb0
    [<c000000000360558>] .kobject_set_name_vargs+0x44/0xb4
    [<c0000000003f06d0>] .dev_set_name+0x50/0x6c
    [<c00000000042a794>] .scsi_sysfs_device_initialize+0xd0/0x16c
    [<c000000000426600>] .scsi_alloc_sdev+0x1c4/0x284
    [<c000000000426b50>] .scsi_probe_and_add_lun+0x144/0xbd4
    [<c0000000004279bc>] .__scsi_scan_target+0xfc/0x658
    [<c000000000427f90>] .scsi_scan_channel+0x78/0xe8
    [<c0000000004280cc>] .scsi_scan_host_selected+0xcc/0x154
    [<c00000000042823c>] .do_scsi_scan_host+0xe8/0x10c
    [<c0000000004286ec>] .scsi_scan_host+0x250/0x294
    [<c000000000456ef8>] .ibmvscsi_probe+0x4f8/0x630
    [<c000000000027418>] .vio_bus_probe+0x360/0x3cc


cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] kmemleak: Allow kmemleak to be built on powerpc

[Josh Boyer]

On Thu, Jul 16, 2009 at 05:43:50PM +1000, Michael Ellerman wrote:
>On Thu, 2009-07-16 at 11:25 +1000, Michael Ellerman wrote:
>> Very lightly tested, doesn't crash the kernel.
>> 
>> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
>> ---
>> 
>> It doesn't look like we actually need to add any support in the
>> arch code - or is there something I'm missing?
>
>Hmm, I think we want to add annotations in lib/lmb.c don't we? That's
>our low-level pre-bootmem allocator.
>
>And we have the same problem with _edata as x86.

I'll point out that the Fedora developers enabled this in the rawhide kernels
for 3 days, and then turned it back off because most of the things found were
false positives.

Might still be worth poking at, but until it gets a bit less buggy itself it
could be a timesink.

josh

Re: [PATCH] kmemleak: Allow kmemleak to be built on powerpc

[Catalin Marinas]

On Thu, 2009-07-16 at 07:31 -0400, Josh Boyer wrote:
> On Thu, Jul 16, 2009 at 05:43:50PM +1000, Michael Ellerman wrote:
> >On Thu, 2009-07-16 at 11:25 +1000, Michael Ellerman wrote:
> >> Very lightly tested, doesn't crash the kernel.
> >> 
> >> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> >> ---
> >> 
> >> It doesn't look like we actually need to add any support in the
> >> arch code - or is there something I'm missing?
> >
> >Hmm, I think we want to add annotations in lib/lmb.c don't we? That's
> >our low-level pre-bootmem allocator.
> >
> >And we have the same problem with _edata as x86.
> 
> I'll point out that the Fedora developers enabled this in the rawhide kernels
> for 3 days, and then turned it back off because most of the things found were
> false positives.

Yes, but with 2.6.31-rc3 the number of false positives dropped
considerably. I don't plan to push any new kmemleak patches for 2.6.31
(probably only a bug-fix).

Anyway, even when it reports real leaks, it is very time consuming to
investigate.

I don't have any PPC hardware but as long as someone sorts out things
like _edata or other PPC-specific allocators which aren't currently
tracked by kmemleak, I'm OK with the original patch:

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

-- 
Catalin

Re: [PATCH] kmemleak: Allow kmemleak to be built on powerpc

[Catalin Marinas]

On Thu, 2009-07-16 at 17:43 +1000, Michael Ellerman wrote:
> On Thu, 2009-07-16 at 11:25 +1000, Michael Ellerman wrote:
> > Very lightly tested, doesn't crash the kernel.
> > 
> > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > ---
> > 
> > It doesn't look like we actually need to add any support in the
> > arch code - or is there something I'm missing?
> 
> Hmm, I think we want to add annotations in lib/lmb.c don't we? That's
> our low-level pre-bootmem allocator.

Yes, I think so (I'm not using this on ARM or x86 so I can't really test
it). Without these hooks, there kmemleak reports aren't that useful
(probably too many).

-- 
Catalin

Re: [PATCH] kmemleak: Allow kmemleak to be built on powerpc

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

On Thu, 2009-07-16 at 18:52 +0100, Catalin Marinas wrote:
> On Thu, 2009-07-16 at 17:43 +1000, Michael Ellerman wrote:
> > On Thu, 2009-07-16 at 11:25 +1000, Michael Ellerman wrote:
> > > Very lightly tested, doesn't crash the kernel.
> > > 
> > > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > > ---
> > > 
> > > It doesn't look like we actually need to add any support in the
> > > arch code - or is there something I'm missing?
> > 
> > Hmm, I think we want to add annotations in lib/lmb.c don't we? That's
> > our low-level pre-bootmem allocator.
> 
> Yes, I think so (I'm not using this on ARM or x86 so I can't really test
> it). Without these hooks, there kmemleak reports aren't that useful
> (probably too many).

The wrinkle is that lmb never frees, so by definition it can't leak :)

But we could have memory allocated with lmb that has pointers to other
objects allocated later, and we want kmemleak to scan the lmb allocated
blocks to find those references.

So the question is do we need to annotate lmb so that will happen, or
does kmemleak scan all kernel memory, regardless of where it's
allocated?

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] kmemleak: Allow kmemleak to be built on powerpc

[Catalin Marinas]

On Fri, 2009-07-17 at 10:29 +1000, Michael Ellerman wrote:
> On Thu, 2009-07-16 at 18:52 +0100, Catalin Marinas wrote:
> > On Thu, 2009-07-16 at 17:43 +1000, Michael Ellerman wrote:
> > > On Thu, 2009-07-16 at 11:25 +1000, Michael Ellerman wrote:
> > > > Very lightly tested, doesn't crash the kernel.
> > > > 
> > > > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > > > ---
> > > > 
> > > > It doesn't look like we actually need to add any support in the
> > > > arch code - or is there something I'm missing?
> > > 
> > > Hmm, I think we want to add annotations in lib/lmb.c don't we? That's
> > > our low-level pre-bootmem allocator.
> > 
> > Yes, I think so (I'm not using this on ARM or x86 so I can't really test
> > it). Without these hooks, there kmemleak reports aren't that useful
> > (probably too many).
> 
> The wrinkle is that lmb never frees, so by definition it can't leak :)

You can pass min_count = 0 to kmemleak_alloc() so that it would never
report such blocks as leaks (see the alloc_bootmem hooks).

> But we could have memory allocated with lmb that has pointers to other
> objects allocated later, and we want kmemleak to scan the lmb allocated
> blocks to find those references.
> 
> So the question is do we need to annotate lmb so that will happen, or
> does kmemleak scan all kernel memory, regardless of where it's
> allocated?

Apart from the data and bss sections, it only scans the memory which was
explicitly allocated. So, you need these hooks in the lmb code.

The mainline kernel scans for the task stacks by going through the full
tasks list. However, traversing this list requires a lock which
increases the latency quite a lot. For the next merging window, I added
hooks to the alloc_thread_info/free_thread_info functions. If the ppc
code has __HAVE_ARCH_THREAD_INFO_ALLOCATOR defined, you may need to add
some hooks in there as well (but note that kmallloc/kmem_cache_alloc are
tracked by kmemleak already, so you only need the hooks if the
thread_info allocator uses __get_free_pages).

-- 
Catalin

Re: [PATCH] kmemleak: Allow kmemleak to be built on powerpc

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2462 bytes --]

On Fri, 2009-07-17 at 09:26 +0100, Catalin Marinas wrote:
> On Fri, 2009-07-17 at 10:29 +1000, Michael Ellerman wrote:
> > On Thu, 2009-07-16 at 18:52 +0100, Catalin Marinas wrote:
> > > On Thu, 2009-07-16 at 17:43 +1000, Michael Ellerman wrote:
> > > > On Thu, 2009-07-16 at 11:25 +1000, Michael Ellerman wrote:
> > > > > Very lightly tested, doesn't crash the kernel.
> > > > > 
> > > > > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > > > > ---
> > > > > 
> > > > > It doesn't look like we actually need to add any support in the
> > > > > arch code - or is there something I'm missing?
> > > > 
> > > > Hmm, I think we want to add annotations in lib/lmb.c don't we? That's
> > > > our low-level pre-bootmem allocator.
> > > 
> > > Yes, I think so (I'm not using this on ARM or x86 so I can't really test
> > > it). Without these hooks, there kmemleak reports aren't that useful
> > > (probably too many).
> > 
> > The wrinkle is that lmb never frees, so by definition it can't leak :)
> 
> You can pass min_count = 0 to kmemleak_alloc() so that it would never
> report such blocks as leaks (see the alloc_bootmem hooks).

OK. I couldn't see any description of what min_count meant anywhere,
I'll try that though.

> > But we could have memory allocated with lmb that has pointers to other
> > objects allocated later, and we want kmemleak to scan the lmb allocated
> > blocks to find those references.
> > 
> > So the question is do we need to annotate lmb so that will happen, or
> > does kmemleak scan all kernel memory, regardless of where it's
> > allocated?
> 
> Apart from the data and bss sections, it only scans the memory which was
> explicitly allocated. So, you need these hooks in the lmb code.

OK, cool, I'll do a patch to add them.

> The mainline kernel scans for the task stacks by going through the full
> tasks list. However, traversing this list requires a lock which
> increases the latency quite a lot. For the next merging window, I added
> hooks to the alloc_thread_info/free_thread_info functions. If the ppc
> code has __HAVE_ARCH_THREAD_INFO_ALLOCATOR defined, you may need to add
> some hooks in there as well (but note that kmallloc/kmem_cache_alloc are
> tracked by kmemleak already, so you only need the hooks if the
> thread_info allocator uses __get_free_pages).

We do have our own allocator, but it just uses a kmem_cache, so that
should be fine.

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] kmemleak: Allow kmemleak to be built on powerpc

[Catalin Marinas]

On Fri, 2009-07-17 at 18:32 +1000, Michael Ellerman wrote:
> On Fri, 2009-07-17 at 09:26 +0100, Catalin Marinas wrote:
> > On Fri, 2009-07-17 at 10:29 +1000, Michael Ellerman wrote:
> > > The wrinkle is that lmb never frees, so by definition it can't leak :)
> > 
> > You can pass min_count = 0 to kmemleak_alloc() so that it would never
> > report such blocks as leaks (see the alloc_bootmem hooks).
> 
> OK. I couldn't see any description of what min_count meant anywhere,

There isn't any, indeed. I'll try to add some description to the
kmemleak api. But it basically means the minimum number a pointer value
should be found during scanning so that it is not reported as a leak. If
this value is 0, it would never be reported. The
kmalloc/kmem_cache_alloc have this value 1 and vmalloc is higher because
of other structures like vm_area holding pointers o the same block.

-- 
Catalin