Re: [PATCH v2] slob: add size header to all allocations

From: Vlastimil Babka
Date: Mon Oct 25 2021 - 05:37:00 EST


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@xxxxxxxxx
> Signed-off-by: Rustam Kovhaev <rkovhaev@xxxxxxxxx>

Seems overal correct to me, thanks! I'll just suggest some improvements:

> ---
> 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`.

> -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);