linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] mm/slab: use kmalloc_node() for off slab freelist_idx_t array allocation
@ 2022-10-15 11:47 Guenter Roeck
  0 siblings, 0 replies; 2+ messages in thread
From: Guenter Roeck @ 2022-10-15 11:47 UTC (permalink / raw)
  To: Hyeonggon Yoo
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, Roman Gushchin, linux-mm,
	linux-kernel

On Sat, Oct 15, 2022 at 01:34:29PM +0900, Hyeonggon Yoo wrote:
> After commit d6a71648dbc0 ("mm/slab: kmalloc: pass requests larger than
> order-1 page to page allocator"), SLAB passes large ( > PAGE_SIZE * 2)
> requests to buddy like SLUB does.
> 
> SLAB has been using kmalloc caches to allocate freelist_idx_t array for
> off slab caches. But after the commit, freelist_size can be bigger than
> KMALLOC_MAX_CACHE_SIZE.
> 
> Instead of using pointer to kmalloc cache, use kmalloc_node() and only
> check if the kmalloc cache is off slab during calculate_slab_order().
> If freelist_size > KMALLOC_MAX_CACHE_SIZE, no looping condition happens
> as it allocates freelist_idx_t array directly from buddy.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: d6a71648dbc0 ("mm/slab: kmalloc: pass requests larger than order-1 page to page allocator")
> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> ---
> 
> @Guenter:
> 	This fixes the issue on my emulation.
> 	Can you please test this on your environment?

Yes, that fixes the problem for me.

Tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks,
Guenter

> 
>  include/linux/slab_def.h |  1 -
>  mm/slab.c                | 37 +++++++++++++++++++------------------
>  2 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/slab_def.h b/include/linux/slab_def.h
> index e24c9aff6fed..f0ffad6a3365 100644
> --- a/include/linux/slab_def.h
> +++ b/include/linux/slab_def.h
> @@ -33,7 +33,6 @@ struct kmem_cache {
>  
>  	size_t colour;			/* cache colouring range */
>  	unsigned int colour_off;	/* colour offset */
> -	struct kmem_cache *freelist_cache;
>  	unsigned int freelist_size;
>  
>  	/* constructor func */
> diff --git a/mm/slab.c b/mm/slab.c
> index a5486ff8362a..d1f6e2c64c2e 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -1619,7 +1619,7 @@ static void slab_destroy(struct kmem_cache *cachep, struct slab *slab)
>  	 * although actual page can be freed in rcu context
>  	 */
>  	if (OFF_SLAB(cachep))
> -		kmem_cache_free(cachep->freelist_cache, freelist);
> +		kfree(freelist);
>  }
>  
>  /*
> @@ -1671,21 +1671,27 @@ static size_t calculate_slab_order(struct kmem_cache *cachep,
>  		if (flags & CFLGS_OFF_SLAB) {
>  			struct kmem_cache *freelist_cache;
>  			size_t freelist_size;
> +			size_t freelist_cache_size;
>  
>  			freelist_size = num * sizeof(freelist_idx_t);
> -			freelist_cache = kmalloc_slab(freelist_size, 0u);
> -			if (!freelist_cache)
> -				continue;
> -
> -			/*
> -			 * Needed to avoid possible looping condition
> -			 * in cache_grow_begin()
> -			 */
> -			if (OFF_SLAB(freelist_cache))
> -				continue;
> +			if (freelist_size > KMALLOC_MAX_CACHE_SIZE) {
> +				freelist_cache_size = PAGE_SIZE << get_order(freelist_size);
> +			} else {
> +				freelist_cache = kmalloc_slab(freelist_size, 0u);
> +				if (!freelist_cache)
> +					continue;
> +				freelist_cache_size = freelist_cache->size;
> +
> +				/*
> +				 * Needed to avoid possible looping condition
> +				 * in cache_grow_begin()
> +				 */
> +				if (OFF_SLAB(freelist_cache))
> +					continue;
> +			}
>  
>  			/* check if off slab has enough benefit */
> -			if (freelist_cache->size > cachep->size / 2)
> +			if (freelist_cache_size > cachep->size / 2)
>  				continue;
>  		}
>  
> @@ -2061,11 +2067,6 @@ int __kmem_cache_create(struct kmem_cache *cachep, slab_flags_t flags)
>  		cachep->flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
>  #endif
>  
> -	if (OFF_SLAB(cachep)) {
> -		cachep->freelist_cache =
> -			kmalloc_slab(cachep->freelist_size, 0u);
> -	}
> -
>  	err = setup_cpu_cache(cachep, gfp);
>  	if (err) {
>  		__kmem_cache_release(cachep);
> @@ -2292,7 +2293,7 @@ static void *alloc_slabmgmt(struct kmem_cache *cachep,
>  		freelist = NULL;
>  	else if (OFF_SLAB(cachep)) {
>  		/* Slab management obj is off-slab. */
> -		freelist = kmem_cache_alloc_node(cachep->freelist_cache,
> +		freelist = kmalloc_node(cachep->freelist_size,
>  					      local_flags, nodeid);
>  	} else {
>  		/* We will use last bytes at the slab for freelist */
> -- 
> 2.32.0


^ permalink raw reply	[flat|nested] 2+ messages in thread
* Re: [PATCH v4 10/17] mm/slab: kmalloc: pass requests larger than order-1 page to page allocator
@ 2022-10-14 20:58 Guenter Roeck
  2022-10-15  4:34 ` [PATCH] mm/slab: use kmalloc_node() for off slab freelist_idx_t array allocation Hyeonggon Yoo
  0 siblings, 1 reply; 2+ messages in thread
From: Guenter Roeck @ 2022-10-14 20:58 UTC (permalink / raw)
  To: Hyeonggon Yoo
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, Roman Gushchin, linux-mm,
	linux-kernel

Hi,

On Wed, Aug 17, 2022 at 07:18:19PM +0900, Hyeonggon Yoo wrote:
> There is not much benefit for serving large objects in kmalloc().
> Let's pass large requests to page allocator like SLUB for better
> maintenance of common code.
> 
> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
> ---

This patch results in a WARNING backtrace in all mips and sparc64
emulations.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at mm/slab_common.c:729 kmalloc_slab+0xc0/0xdc
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 6.0.0-11990-g9c9155a3509a #1
Stack : ffffffff 801b2a18 80dd0000 00000004 00000000 00000000 81023cd4 00000000
        81040000 811a9930 81040000 8104a628 81101833 00000001 81023c78 00000000
        00000000 00000000 80f5d858 81023b98 00000001 00000023 00000000 ffffffff
        00000000 00000064 00000002 81040000 81040000 00000001 80f5d858 000002d9
        00000000 00000000 80000000 80002000 00000000 00000000 00000000 00000000
        ...
Call Trace:
[<8010a2bc>] show_stack+0x38/0x118
[<80cf5f7c>] dump_stack_lvl+0xac/0x104
[<80130d7c>] __warn+0xe0/0x224
[<80cdba5c>] warn_slowpath_fmt+0x64/0xb8
[<8028c058>] kmalloc_slab+0xc0/0xdc

irq event stamp: 0
hardirqs last  enabled at (0): [<00000000>] 0x0
hardirqs last disabled at (0): [<00000000>] 0x0
softirqs last  enabled at (0): [<00000000>] 0x0
softirqs last disabled at (0): [<00000000>] 0x0
---[ end trace 0000000000000000 ]---

Guenter


^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-10-15 11:47 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-15 11:47 [PATCH] mm/slab: use kmalloc_node() for off slab freelist_idx_t array allocation Guenter Roeck
  -- strict thread matches above, loose matches on Subject: below --
2022-10-14 20:58 [PATCH v4 10/17] mm/slab: kmalloc: pass requests larger than order-1 page to page allocator Guenter Roeck
2022-10-15  4:34 ` [PATCH] mm/slab: use kmalloc_node() for off slab freelist_idx_t array allocation Hyeonggon Yoo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).