Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 29 Aug 2022 12:18:57 +0900
From:   Hyeonggon Yoo <42.hyeyoo@gmail.com>
To:     Dawei Li <set_pte_at@outlook.com>
Cc:     cl@linux.com, penberg@kernel.org, rientjes@google.com,
        iamjoonsoo.kim@lge.com, akpm@linux-foundation.org, vbabka@suse.cz,
        roman.gushchin@linux.dev, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm: simplify size2index conversion of __kmalloc_index
Message-ID: <YwwwIcZ6FUy6J4xh@hyeyoo>
References: <TYCP286MB2323E622C54B765F087C073ECA779@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <TYCP286MB2323E622C54B765F087C073ECA779@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
Precedence: bulk

On Sun, Aug 28, 2022 at 11:14:48PM +0800, Dawei Li wrote:
> Current size2index is implemented by one to one hardcode mapping,
> which can be improved by order_base_2().
> Must be careful to not violate compile-time optimization rule.

> Generated code for caller of kmalloc:
> 48 8b 3d 9f 0b 6b 01 mov    0x16b0b9f(%rip),%rdi
>                             # ffffffff826d1568 <kmalloc_caches+0x48>
> ba 08 01 00 00       mov    $0x108,%edx
> be c0 0d 00 00       mov    $0xdc0,%esi
> e8 98 d7 2e 00       callq  ffffffff8130e170 <kmem_cache_alloc_trace>
> 
> Signed-off-by: Dawei Li <set_pte_at@outlook.com>
> ---
>  include/linux/slab.h | 34 +++++++++-------------------------
>  1 file changed, 9 insertions(+), 25 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 0fefdf528e0d..66452a4357c6 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -17,7 +17,7 @@
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
>  #include <linux/percpu-refcount.h>
> -
> +#include <linux/log2.h>
>  
>  /*
>   * Flags to pass to kmem_cache_create().
> @@ -394,31 +394,16 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
>  
>  	if (KMALLOC_MIN_SIZE <= 32 && size > 64 && size <= 96)
>  		return 1;
> +
>  	if (KMALLOC_MIN_SIZE <= 64 && size > 128 && size <= 192)
>  		return 2;
> -	if (size <=          8) return 3;
> -	if (size <=         16) return 4;
> -	if (size <=         32) return 5;
> -	if (size <=         64) return 6;
> -	if (size <=        128) return 7;
> -	if (size <=        256) return 8;
> -	if (size <=        512) return 9;
> -	if (size <=       1024) return 10;
> -	if (size <=   2 * 1024) return 11;
> -	if (size <=   4 * 1024) return 12;
> -	if (size <=   8 * 1024) return 13;
> -	if (size <=  16 * 1024) return 14;
> -	if (size <=  32 * 1024) return 15;
> -	if (size <=  64 * 1024) return 16;
> -	if (size <= 128 * 1024) return 17;
> -	if (size <= 256 * 1024) return 18;
> -	if (size <= 512 * 1024) return 19;
> -	if (size <= 1024 * 1024) return 20;
> -	if (size <=  2 * 1024 * 1024) return 21;
> -	if (size <=  4 * 1024 * 1024) return 22;
> -	if (size <=  8 * 1024 * 1024) return 23;
> -	if (size <=  16 * 1024 * 1024) return 24;
> -	if (size <=  32 * 1024 * 1024) return 25;

It does not apply. better rebase it on Vlastimil's slab tree (for-next branch)
https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/

> +
> +	if (size <= 8)
> +		return 3;
> +
> +	/* Following compile-time optimization rule is mandatory. */
> +	if (size <= 32 * 1024 * 1024)
> +		return order_base_2(size);

Oh, it seems order_base_2() does compile-time opitmization as well.

With order_base_2(), what about using KMALLOC_MAX_CACHE_SIZE instead of 32 * 1024 * 1024?
I think that would be more robust.

Hmm also better check if it works okay with kfence tests (it passes non-constant value)
let't check if it breaks after rebase.

Thanks!

>  
>  	if (!IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) && size_is_constant)
>  		BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
> @@ -700,7 +685,6 @@ static inline __alloc_size(1, 2) void *kcalloc_node(size_t n, size_t size, gfp_t
>  	return kmalloc_array_node(n, size, flags | __GFP_ZERO, node);
>  }
>  
> -
>  #ifdef CONFIG_NUMA
>  extern void *__kmalloc_node_track_caller(size_t size, gfp_t flags, int node,
>  					 unsigned long caller) __alloc_size(1);
> -- 
> 2.25.1
> 

-- 
Thanks,
Hyeonggon