| Message ID | f18967360113efe4354a9ad0d71ead9a0f7579ea.1247707485.git.michael@ellerman.id.au (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | bbdc16f58ed84523e8991f103dceb586e9053e04 |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
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
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
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>
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).
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
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).
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
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.
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
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(-)