| Message ID | 20200627045507.GA57675@lilong (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] mm:free unused pages in kmalloc_order | expand |
On Sat, Jun 27, 2020 at 04:55:07AM +0000, Long Li wrote: > Environment using the slub allocator, 1G memory in my ARM32. > kmalloc(1024, GFP_HIGHUSER) can allocate memory normally, > kmalloc(64*1024, GFP_HIGHUSER) will cause a memory leak, because > alloc_pages returns highmem physical pages, but it cannot be directly > converted into a virtual address and return NULL, the pages has not > been released. Usually driver developers will not use the > GFP_HIGHUSER flag to allocate memory in kmalloc, but I think this > memory leak is not perfect, it is best to be fixed. This is the > first time I have posted a patch, there may be something wrong. Slab used to disallow GFP_HIGHMEM allocations earlier than this, so you'd never get here. See one moron's patch here, 20 years ago ... https://lore.kernel.org/lkml/20001228145801.C19693@parcelfarce.linux.theplanet.co.uk/ Things changed a bit since then. SLAB_LEVEL_MASK became GFP_SLAB_BUG_MASK, then GFP_SLAB_BUG_MASK moved to mm/internal.h. Nowadays, GFP_SLAB_BUG_MASK is checked only in new_slab(), and it is definitely skipped when we go through the kmalloc_large() patch. Could you send a replacement patch which checks GFP_SLAB_BUG_MASK before calling alloc_pages()? > +++ b/mm/slab_common.c > @@ -819,8 +819,12 @@ void *kmalloc_order(size_t size, gfp_t flags, unsigned int order) > page = alloc_pages(flags, order); > if (likely(page)) { > ret = page_address(page); > - mod_node_page_state(page_pgdat(page), NR_SLAB_UNRECLAIMABLE_B, > - PAGE_SIZE << order); > + if (ret) > + mod_node_page_state(page_pgdat(page), > + NR_SLAB_UNRECLAIMABLE_B, > + PAGE_SIZE << order); > + else > + __free_pages(page, order); > } > ret = kasan_kmalloc_large(ret, size, flags); > /* As ret might get tagged, call kmemleak hook after KASAN. */ > -- > 2.17.1 > >
On Sat, 27 Jun 2020, Long Li wrote: > Environment using the slub allocator, 1G memory in my ARM32. > kmalloc(1024, GFP_HIGHUSER) can allocate memory normally, > kmalloc(64*1024, GFP_HIGHUSER) will cause a memory leak, because > alloc_pages returns highmem physical pages, but it cannot be directly > converted into a virtual address and return NULL, the pages has not > been released. Usually driver developers will not use the > GFP_HIGHUSER flag to allocate memory in kmalloc, but I think this > memory leak is not perfect, it is best to be fixed. This is the > first time I have posted a patch, there may be something wrong. Highmem is not supported by the slab allocators. Please ensure that there is a warning generated if someone attempts to do such an allocation. We used to check for that. In order to make such allocations possible one would have to create yet another kmalloc array for high memory.
On Mon, 29 Jun 2020, Matthew Wilcox wrote:
> Slab used to disallow GFP_HIGHMEM allocations earlier than this,
It is still not allowed and not supported.
On Mon, Jun 29, 2020 at 02:48:06PM +0000, Christopher Lameter wrote: > On Sat, 27 Jun 2020, Long Li wrote: > > Environment using the slub allocator, 1G memory in my ARM32. > > kmalloc(1024, GFP_HIGHUSER) can allocate memory normally, > > kmalloc(64*1024, GFP_HIGHUSER) will cause a memory leak, because > > alloc_pages returns highmem physical pages, but it cannot be directly > > converted into a virtual address and return NULL, the pages has not > > been released. Usually driver developers will not use the > > GFP_HIGHUSER flag to allocate memory in kmalloc, but I think this > > memory leak is not perfect, it is best to be fixed. This is the > > first time I have posted a patch, there may be something wrong. > > Highmem is not supported by the slab allocators. Please ensure that there > is a warning generated if someone attempts to do such an allocation. We > used to check for that. Sounds like we need a test somewhere that checks this behaviour. > In order to make such allocations possible one would have to create yet > another kmalloc array for high memory. Not for this case because it goes straight to kmalloc_order(). What does make this particular case impossible is that we can't kmap() a compound page. We could vmap it, but why are we bothering?
On Mon, 29 Jun 2020, Matthew Wilcox wrote: > Sounds like we need a test somewhere that checks this behaviour. > > > In order to make such allocations possible one would have to create yet > > another kmalloc array for high memory. > > Not for this case because it goes straight to kmalloc_order(). What does > make this particular case impossible is that we can't kmap() a compound > page. We could vmap it, but why are we bothering? Well yes it will work if the slab allocator falls back to the page allocator. Higher order allocation through kmalloc ;-). How much fun and uselessness .... Why not call the page allocator directly and play with all the bits you want? Any regular small object allocation with GFP_HIGH will lead to strange effects if the bit is not checked.
diff --git a/mm/slab_common.c b/mm/slab_common.c index a143a8c8f874..d2c53b980ab3 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -819,8 +819,12 @@ void *kmalloc_order(size_t size, gfp_t flags, unsigned int order) page = alloc_pages(flags, order); if (likely(page)) { ret = page_address(page); - mod_node_page_state(page_pgdat(page), NR_SLAB_UNRECLAIMABLE_B, - PAGE_SIZE << order); + if (ret) + mod_node_page_state(page_pgdat(page), + NR_SLAB_UNRECLAIMABLE_B, + PAGE_SIZE << order); + else + __free_pages(page, order); } ret = kasan_kmalloc_large(ret, size, flags); /* As ret might get tagged, call kmemleak hook after KASAN. */
Environment using the slub allocator, 1G memory in my ARM32. kmalloc(1024, GFP_HIGHUSER) can allocate memory normally, kmalloc(64*1024, GFP_HIGHUSER) will cause a memory leak, because alloc_pages returns highmem physical pages, but it cannot be directly converted into a virtual address and return NULL, the pages has not been released. Usually driver developers will not use the GFP_HIGHUSER flag to allocate memory in kmalloc, but I think this memory leak is not perfect, it is best to be fixed. This is the first time I have posted a patch, there may be something wrong. Signed-off-by: Long Li <lonuxli.64@gmail.com> --- mm/slab_common.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)