| Message ID | 1442169397-4271-1-git-send-email-kda@linux-powerpc.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, 2015-09-13 at 21:36 +0300, Denis Kirjanov wrote: > During the MSI bitmap test on boot kmemleak spews the following trace: > > unreferenced object 0xc00000016e86c900 (size 64): > comm "swapper/0", pid 1, jiffies 4294893173 (age 518.024s) > hex dump (first 32 bytes): > 00 00 01 ff 7f ff 7f 37 00 00 00 00 00 00 00 00 > .......7........ > ff ff ff ff ff ff ff ff 01 ff ff ff ff ff ff ff > ................ > backtrace: > [<c00000000003eebc>] .zalloc_maybe_bootmem+0x3c/0x380 > [<c000000000042d6c>] .msi_bitmap_alloc+0x3c/0xb0 > [<c000000000a9aff8>] > .msi_bitmap_selftest+0x30/0x2b4 > [<c0000000000090f4>] > .do_one_initcall+0xd4/0x270 > [<c000000000a8e250>] > .kernel_init_freeable+0x1a0/0x280 > [<c000000000009b5c>] > .kernel_init+0x1c/0x120 > [<c000000000007fbc>] > .ret_from_kernel_thread+0x58/0x9c > > The comment in msi_bitmap_free() states that we can't free > the bitmap so mark it with the kmemleak_not_leak(). Yeah, and I've always hated that comment :) I assume it's still true now that we don't use bootmem? cheers
On Mon, Sep 14, 2015 at 04:04:37PM +1000, Michael Ellerman wrote: > On Sun, 2015-09-13 at 21:36 +0300, Denis Kirjanov wrote: > > During the MSI bitmap test on boot kmemleak spews the following trace: > > > > unreferenced object 0xc00000016e86c900 (size 64): > > comm "swapper/0", pid 1, jiffies 4294893173 (age 518.024s) > > hex dump (first 32 bytes): > > 00 00 01 ff 7f ff 7f 37 00 00 00 00 00 00 00 00 > > .......7........ > > ff ff ff ff ff ff ff ff 01 ff ff ff ff ff ff ff > > ................ > > backtrace: > > [<c00000000003eebc>] .zalloc_maybe_bootmem+0x3c/0x380 > > [<c000000000042d6c>] .msi_bitmap_alloc+0x3c/0xb0 > > [<c000000000a9aff8>] > > .msi_bitmap_selftest+0x30/0x2b4 > > [<c0000000000090f4>] > > .do_one_initcall+0xd4/0x270 > > [<c000000000a8e250>] > > .kernel_init_freeable+0x1a0/0x280 > > [<c000000000009b5c>] > > .kernel_init+0x1c/0x120 > > [<c000000000007fbc>] > > .ret_from_kernel_thread+0x58/0x9c > > > > The comment in msi_bitmap_free() states that we can't free > > the bitmap so mark it with the kmemleak_not_leak(). > > Yeah, and I've always hated that comment :) > > I assume it's still true now that we don't use bootmem? If it's not bootmem, it's probably memblock. Looking at the code, it seems that msi_bitmap_free() cannot make the distinction between a slab allocation and a bootmem/memblock one (allocated via zalloc_maybe_bootmem). You could add some flag to struct msi_bitmap based on mem_init_done to be able to reclaim some slab memory later. If the bitmap is small and such allocation doesn't happen outside boot, it may not be worth the effort.
On Mon, 2015-09-14 at 10:15 +0100, Catalin Marinas wrote: > On Mon, Sep 14, 2015 at 04:04:37PM +1000, Michael Ellerman wrote: > > On Sun, 2015-09-13 at 21:36 +0300, Denis Kirjanov wrote: > > > The comment in msi_bitmap_free() states that we can't free > > > the bitmap so mark it with the kmemleak_not_leak(). > > > > Yeah, and I've always hated that comment :) > > > > I assume it's still true now that we don't use bootmem? > > If it's not bootmem, it's probably memblock. Looking at the code, it > seems that msi_bitmap_free() cannot make the distinction between a slab > allocation and a bootmem/memblock one (allocated via > zalloc_maybe_bootmem). Yeah it is memblock these days. > You could add some flag to struct msi_bitmap based on mem_init_done to > be able to reclaim some slab memory later. If the bitmap is small and > such allocation doesn't happen outside boot, it may not be worth the > effort. Right, I think that's the only solution, and it's not quite worth the trouble because it's generally not freed at all, except via error paths. Still I think it would be better to move the kmemleak annotation into the msi bitmap test code, or maybe a wrapper that's called by the test code. That way if someone starts calling alloc/free from a hotplug path for example, kmemleak will still notice that free isn't really freeing. cheers
On Mon, Sep 14, 2015 at 07:36:49PM +1000, Michael Ellerman wrote: > On Mon, 2015-09-14 at 10:15 +0100, Catalin Marinas wrote: > > You could add some flag to struct msi_bitmap based on mem_init_done to > > be able to reclaim some slab memory later. If the bitmap is small and > > such allocation doesn't happen outside boot, it may not be worth the > > effort. > > Right, I think that's the only solution, and it's not quite worth the trouble > because it's generally not freed at all, except via error paths. > > Still I think it would be better to move the kmemleak annotation into the msi > bitmap test code, or maybe a wrapper that's called by the test code. Other kmemleak annotations throughout the kernel are usually added immediately after the allocation place (since that's what kmemleak reports as a leak). > That way if someone starts calling alloc/free from a hotplug path for example, > kmemleak will still notice that free isn't really freeing. Kmemleak would only notice the moment you clear the last reference to the allocated memory block (like bmp->bitmap = NULL), so this patch should work as along as "freeing" is done via msi_bitmap_free(). BTW, you can even use kmemleak_ignore(). The difference is that the bitmap won't be scanned by kmemleak and that's fine since it doesn't contain any pointers.
On 9/14/15, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Mon, Sep 14, 2015 at 07:36:49PM +1000, Michael Ellerman wrote: >> On Mon, 2015-09-14 at 10:15 +0100, Catalin Marinas wrote: >> > You could add some flag to struct msi_bitmap based on mem_init_done to >> > be able to reclaim some slab memory later. If the bitmap is small and >> > such allocation doesn't happen outside boot, it may not be worth the >> > effort. >> >> Right, I think that's the only solution, and it's not quite worth the >> trouble >> because it's generally not freed at all, except via error paths. >> >> Still I think it would be better to move the kmemleak annotation into the >> msi >> bitmap test code, or maybe a wrapper that's called by the test code. > > Other kmemleak annotations throughout the kernel are usually added > immediately after the allocation place (since that's what kmemleak > reports as a leak). > >> That way if someone starts calling alloc/free from a hotplug path for >> example, >> kmemleak will still notice that free isn't really freeing. > > Kmemleak would only notice the moment you clear the last reference to > the allocated memory block (like bmp->bitmap = NULL), so this patch > should work as along as "freeing" is done via msi_bitmap_free(). Ok, I'll move kmemleak annotaion for the bitmap inside the test_basics() plus add a flag into msi_bitmap to test for the type of allocation though the slab_is_available() Thank you. > > BTW, you can even use kmemleak_ignore(). The difference is that the > bitmap won't be scanned by kmemleak and that's fine since it doesn't > contain any pointers. > > -- > Catalin > >
diff --git a/arch/powerpc/sysdev/msi_bitmap.c b/arch/powerpc/sysdev/msi_bitmap.c index 73b64c7..bd9fa54 100644 --- a/arch/powerpc/sysdev/msi_bitmap.c +++ b/arch/powerpc/sysdev/msi_bitmap.c @@ -139,6 +139,7 @@ int msi_bitmap_alloc(struct msi_bitmap *bmp, unsigned int irq_count, void msi_bitmap_free(struct msi_bitmap *bmp) { /* we can't free the bitmap we don't know if it's bootmem etc. */ + kmemleak_not_leak(bmp->bitmap); of_node_put(bmp->of_node); bmp->bitmap = NULL; }
During the MSI bitmap test on boot kmemleak spews the following trace: unreferenced object 0xc00000016e86c900 (size 64): comm "swapper/0", pid 1, jiffies 4294893173 (age 518.024s) hex dump (first 32 bytes): 00 00 01 ff 7f ff 7f 37 00 00 00 00 00 00 00 00 .......7........ ff ff ff ff ff ff ff ff 01 ff ff ff ff ff ff ff ................ backtrace: [<c00000000003eebc>] .zalloc_maybe_bootmem+0x3c/0x380 [<c000000000042d6c>] .msi_bitmap_alloc+0x3c/0xb0 [<c000000000a9aff8>] .msi_bitmap_selftest+0x30/0x2b4 [<c0000000000090f4>] .do_one_initcall+0xd4/0x270 [<c000000000a8e250>] .kernel_init_freeable+0x1a0/0x280 [<c000000000009b5c>] .kernel_init+0x1c/0x120 [<c000000000007fbc>] .ret_from_kernel_thread+0x58/0x9c The comment in msi_bitmap_free() states that we can't free the bitmap so mark it with the kmemleak_not_leak(). Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org> --- arch/powerpc/sysdev/msi_bitmap.c | 1 + 1 file changed, 1 insertion(+)