| Message ID | 1442337248-4295-1-git-send-email-kda@linux-powerpc.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Sep 15, 2015 at 08:14:08PM +0300, Denis Kirjanov wrote: > diff --git a/arch/powerpc/include/asm/msi_bitmap.h b/arch/powerpc/include/asm/msi_bitmap.h > index 97ac3f4..9a1d2fb 100644 > --- a/arch/powerpc/include/asm/msi_bitmap.h > +++ b/arch/powerpc/include/asm/msi_bitmap.h > @@ -19,6 +19,7 @@ struct msi_bitmap { > unsigned long *bitmap; > spinlock_t lock; > unsigned int irq_count; > + bool bitmap_from_slab; Nitpick: same alignment for bitmap_from_slab with irq_count etc. (unless it's just my mail client). > }; > > int msi_bitmap_alloc_hwirqs(struct msi_bitmap *bmp, int num); > diff --git a/arch/powerpc/sysdev/msi_bitmap.c b/arch/powerpc/sysdev/msi_bitmap.c > index 73b64c7..305ebe3 100644 > --- a/arch/powerpc/sysdev/msi_bitmap.c > +++ b/arch/powerpc/sysdev/msi_bitmap.c > @@ -11,6 +11,7 @@ > #include <linux/slab.h> > #include <linux/kernel.h> > #include <linux/bitmap.h> > +#include <linux/bootmem.h> > #include <asm/msi_bitmap.h> > #include <asm/setup.h> > > @@ -122,7 +123,12 @@ int msi_bitmap_alloc(struct msi_bitmap *bmp, unsigned int irq_count, > size = BITS_TO_LONGS(irq_count) * sizeof(long); > pr_debug("msi_bitmap: allocator bitmap size is 0x%x bytes\n", size); > > - bmp->bitmap = zalloc_maybe_bootmem(size, GFP_KERNEL); > + if (slab_is_available()) { > + bmp->bitmap = kzalloc(size, GFP_KERNEL); > + bmp->bitmap_from_slab = true; > + } else > + bmp->bitmap = memblock_virt_alloc(size, 0); I don't think bmp->bitmap_from_slab is always initialised, so you need to explicitly set it to false here. > + > if (!bmp->bitmap) { > pr_debug("msi_bitmap: ENOMEM allocating allocator bitmap!\n"); > return -ENOMEM; > @@ -138,7 +144,8 @@ 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. */ > + if (bmp->bitmap_from_slab) > + kfree(bmp->bitmap); > of_node_put(bmp->of_node); > bmp->bitmap = NULL; > } > @@ -200,11 +207,11 @@ static void __init test_basics(void) > WARN_ON(rc < 0 && rc % 128 != 0); > > msi_bitmap_free(&bmp); > + if (!bmp.bitmap_from_slab) > + kmemleak_not_leak(bmp.bitmap); As I mentioned in the other thread, I think you can call kmemleak_not_leak() immediately after memblock_virt_alloc() (together with a comment that this is never going to be freed). That would match the other kmemleak API uses throughout the kernel.
On 9/16/15, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Tue, Sep 15, 2015 at 08:14:08PM +0300, Denis Kirjanov wrote: >> diff --git a/arch/powerpc/include/asm/msi_bitmap.h >> b/arch/powerpc/include/asm/msi_bitmap.h >> index 97ac3f4..9a1d2fb 100644 >> --- a/arch/powerpc/include/asm/msi_bitmap.h >> +++ b/arch/powerpc/include/asm/msi_bitmap.h >> @@ -19,6 +19,7 @@ struct msi_bitmap { >> unsigned long *bitmap; >> spinlock_t lock; >> unsigned int irq_count; >> + bool bitmap_from_slab; > > Nitpick: same alignment for bitmap_from_slab with irq_count etc. (unless > it's just my mail client). > >> }; >> >> int msi_bitmap_alloc_hwirqs(struct msi_bitmap *bmp, int num); >> diff --git a/arch/powerpc/sysdev/msi_bitmap.c >> b/arch/powerpc/sysdev/msi_bitmap.c >> index 73b64c7..305ebe3 100644 >> --- a/arch/powerpc/sysdev/msi_bitmap.c >> +++ b/arch/powerpc/sysdev/msi_bitmap.c >> @@ -11,6 +11,7 @@ >> #include <linux/slab.h> >> #include <linux/kernel.h> >> #include <linux/bitmap.h> >> +#include <linux/bootmem.h> >> #include <asm/msi_bitmap.h> >> #include <asm/setup.h> >> >> @@ -122,7 +123,12 @@ int msi_bitmap_alloc(struct msi_bitmap *bmp, unsigned >> int irq_count, >> size = BITS_TO_LONGS(irq_count) * sizeof(long); >> pr_debug("msi_bitmap: allocator bitmap size is 0x%x bytes\n", size); >> >> - bmp->bitmap = zalloc_maybe_bootmem(size, GFP_KERNEL); >> + if (slab_is_available()) { >> + bmp->bitmap = kzalloc(size, GFP_KERNEL); >> + bmp->bitmap_from_slab = true; >> + } else >> + bmp->bitmap = memblock_virt_alloc(size, 0); > > I don't think bmp->bitmap_from_slab is always initialised, so you need > to explicitly set it to false here. Oops, right. > >> + >> if (!bmp->bitmap) { >> pr_debug("msi_bitmap: ENOMEM allocating allocator bitmap!\n"); >> return -ENOMEM; >> @@ -138,7 +144,8 @@ 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. */ >> + if (bmp->bitmap_from_slab) >> + kfree(bmp->bitmap); >> of_node_put(bmp->of_node); >> bmp->bitmap = NULL; >> } >> @@ -200,11 +207,11 @@ static void __init test_basics(void) >> WARN_ON(rc < 0 && rc % 128 != 0); >> >> msi_bitmap_free(&bmp); >> + if (!bmp.bitmap_from_slab) >> + kmemleak_not_leak(bmp.bitmap); > > As I mentioned in the other thread, I think you can call > kmemleak_not_leak() immediately after memblock_virt_alloc() (together > with a comment that this is never going to be freed). That would match > the other kmemleak API uses throughout the kernel. Ah, ok. Thanks for the review! > > -- > Catalin > >
diff --git a/arch/powerpc/include/asm/msi_bitmap.h b/arch/powerpc/include/asm/msi_bitmap.h index 97ac3f4..9a1d2fb 100644 --- a/arch/powerpc/include/asm/msi_bitmap.h +++ b/arch/powerpc/include/asm/msi_bitmap.h @@ -19,6 +19,7 @@ struct msi_bitmap { unsigned long *bitmap; spinlock_t lock; unsigned int irq_count; + bool bitmap_from_slab; }; int msi_bitmap_alloc_hwirqs(struct msi_bitmap *bmp, int num); diff --git a/arch/powerpc/sysdev/msi_bitmap.c b/arch/powerpc/sysdev/msi_bitmap.c index 73b64c7..305ebe3 100644 --- a/arch/powerpc/sysdev/msi_bitmap.c +++ b/arch/powerpc/sysdev/msi_bitmap.c @@ -11,6 +11,7 @@ #include <linux/slab.h> #include <linux/kernel.h> #include <linux/bitmap.h> +#include <linux/bootmem.h> #include <asm/msi_bitmap.h> #include <asm/setup.h> @@ -122,7 +123,12 @@ int msi_bitmap_alloc(struct msi_bitmap *bmp, unsigned int irq_count, size = BITS_TO_LONGS(irq_count) * sizeof(long); pr_debug("msi_bitmap: allocator bitmap size is 0x%x bytes\n", size); - bmp->bitmap = zalloc_maybe_bootmem(size, GFP_KERNEL); + if (slab_is_available()) { + bmp->bitmap = kzalloc(size, GFP_KERNEL); + bmp->bitmap_from_slab = true; + } else + bmp->bitmap = memblock_virt_alloc(size, 0); + if (!bmp->bitmap) { pr_debug("msi_bitmap: ENOMEM allocating allocator bitmap!\n"); return -ENOMEM; @@ -138,7 +144,8 @@ 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. */ + if (bmp->bitmap_from_slab) + kfree(bmp->bitmap); of_node_put(bmp->of_node); bmp->bitmap = NULL; } @@ -200,11 +207,11 @@ static void __init test_basics(void) WARN_ON(rc < 0 && rc % 128 != 0); msi_bitmap_free(&bmp); + if (!bmp.bitmap_from_slab) + kmemleak_not_leak(bmp.bitmap); /* Clients may WARN_ON bitmap == NULL for "not-allocated" */ WARN_ON(bmp.bitmap != NULL); - - kfree(bmp.bitmap); } static void __init test_of_node(void)
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 Added a flag to msi_bitmap for tracking allocations from slab and memblock so we can properly free/handle memory in msi_bitmap_free(). Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org> Changes vor v2: - added a flag to msi_bitmap - kmemleak annotaion moved under CONFIG_MSI_BITMAP_SELFTEST --- arch/powerpc/include/asm/msi_bitmap.h | 1 + arch/powerpc/sysdev/msi_bitmap.c | 15 +++++++++++---- 2 files changed, 12 insertions(+), 4 deletions(-)