Re: [PATCH v2] slob: add size header to all allocations
From: Vlastimil Babka <hidden>
Date: 2021-10-25 09:37:00
Also in:
linux-mm, lkml
On 10/23/21 08:41, Rustam Kovhaev wrote:
Let's prepend both kmalloc() and kmem_cache_alloc() allocations with the size header. It simplifies the slab API and guarantees that both kmem_cache_alloc() and kmalloc() memory could be freed by kfree(). meminfo right after the system boot, without the patch: Slab: 35456 kB the same, with the patch: Slab: 36160 kB Link: https://lore.kernel.org/lkml/20210929212347.1139666-1-rkovhaev@gmail.com (local) Signed-off-by: Rustam Kovhaev <redacted>
Seems overal correct to me, thanks! I'll just suggest some improvements:
quoted hunk ↗ jump to hunk
--- v2: - Allocate compound pages in slob_alloc_node() - Use slob_free_pages() in kfree() - Update documentation Documentation/core-api/memory-allocation.rst | 4 +- mm/slob.c | 114 +++++++++---------- 2 files changed, 55 insertions(+), 63 deletions(-)diff --git a/Documentation/core-api/memory-allocation.rst b/Documentation/core-api/memory-allocation.rst index 5954ddf6ee13..fea0ed11a7c5 100644 --- a/Documentation/core-api/memory-allocation.rst +++ b/Documentation/core-api/memory-allocation.rst@@ -172,5 +172,5 @@ wrappers can allocate memory from that cache. When the allocated memory is no longer needed it must be freed. You can use kvfree() for the memory allocated with `kmalloc`, `vmalloc` and -`kvmalloc`. The slab caches should be freed with kmem_cache_free(). And -don't forget to destroy the cache with kmem_cache_destroy(). +`kvmalloc`. The slab caches can be freed with kmem_cache_free() or kvfree(). +And don't forget to destroy the cache with kmem_cache_destroy().
I would phrase it like this (improves also weird wording "The slab caches should be freed with..." prior to your patch, etc.): When the allocated memory is no longer needed it must be freed. Objects allocated by `kmalloc` can be freed by `kfree` or `kvfree`. Objects allocated by `kmem_cache_alloc` can be freed with `kmem_cache_free` or also by `kfree` or `kvfree`. Memory allocated by `vmalloc` can be freed with `vfree` or `kvfree`. Memory allocated by `kvmalloc` can be freed with `kvfree`. Caches created by `kmem_cache_create` should be freed with with `kmem_cache_destroy`.
quoted hunk ↗ jump to hunk
-static void slob_free_pages(void *b, int order) +static void slob_free_pages(struct page *sp, int order) { - struct page *sp = virt_to_page(b); - - if (current->reclaim_state) - current->reclaim_state->reclaimed_slab += 1 << order; + if (PageSlab(sp)) { + __ClearPageSlab(sp); + page_mapcount_reset(sp); + if (current->reclaim_state) + current->reclaim_state->reclaimed_slab += 1 << order; + } mod_node_page_state(page_pgdat(sp), NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order));@@ -247,9 +244,7 @@ static void *slob_page_alloc(struct page *sp, size_t size, int align, /* * 'aligned' will hold the address of the slob block so that the * address 'aligned'+'align_offset' is aligned according to the - * 'align' parameter. This is for kmalloc() which prepends the - * allocated block with its size, so that the block itself is - * aligned when needed. + * 'align' parameter. */ if (align) { aligned = (slob_t *)@@ -373,25 +368,28 @@ static void *slob_alloc(size_t size, gfp_t gfp, int align, int node, } if (unlikely(gfp & __GFP_ZERO)) memset(b, 0, size); + /* Write size in the header */ + *(unsigned int *)b = size - align_offset; + b = (void *)b + align_offset; return b;
I would just "return (void *)b + align_offset;" here, no need to update 'b'.
}
/*
* slob_free: entry point into the slob allocator.
*/
-static void slob_free(void *block, int size)
+static void slob_free(void *block)
{
struct page *sp;
- slob_t *prev, *next, *b = (slob_t *)block;
+ int align_offset = max_t(size_t, ARCH_KMALLOC_MINALIGN, ARCH_SLAB_MINALIGN);This patch adds a number of these in several functions, it was just __do_kmalloc_node(). It's compile-time constant so I would just #define it somewhere at the top of slob.c, e.g. something like: #if ARCH_KMALLOC_MINALIGN < ARCH_SLAB_MINALIGN #define SLOB_HDR_SIZE ARCH_SLAB_MINALIGN #else #define SLOB_HDR_SIZE ARCH_KMALLOC_MINALIGN #endif
+ void *hdr = block - align_offset; + unsigned int size = *(unsigned int *)hdr + align_offset; + slob_t *prev, *next, *b = hdr;
IMHO this is too subtle to put in the declaration. I would move these assignments below the declarations. That way you can also ditch 'hdr' and just do a 'block -= SLOB_HDR_SIZE;';
slobidx_t units; unsigned long flags; struct list_head *slob_list; - if (unlikely(ZERO_OR_NULL_PTR(block))) - return; - BUG_ON(!size); - - sp = virt_to_page(block); + BUG_ON(!size || size >= PAGE_SIZE); + sp = virt_to_page(hdr); units = SLOB_UNITS(size); spin_lock_irqsave(&slob_lock, flags);