[PATCH] mm: kmalloc_index: remove case when size is more than 32MB

[PATCH] mm: kmalloc_index: remove case when size is more than 32MB

[Hyeonggon Yoo]

the return value of kmalloc_index is used as index of kmalloc_caches,
which is defined as:
  struct kmem_cache *
  kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1]

and KMALLOC_SHIFT_HIGH is defined as:
  #define KMALLOC_SHIFT_HIGH    ((MAX_ORDER + PAGE_SHIFT - 1) <= 25 ? \
                              (MAX_ORDER + PAGE_SHIFT - 1) : 25)

KMALLOC_SHIFT_HIGH is maximum 25 by its definition. thus index of
kmalloc_caches cannot be 26. so this case should be removed.

Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 include/linux/slab.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 0c97d788762c..4694b1db4cb2 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -382,7 +382,6 @@ static __always_inline unsigned int kmalloc_index(size_t size)
 	if (size <=  8 * 1024 * 1024) return 23;
 	if (size <=  16 * 1024 * 1024) return 24;
 	if (size <=  32 * 1024 * 1024) return 25;
-	if (size <=  64 * 1024 * 1024) return 26;
 	BUG();
 
 	/* Will never be reached. Needed because the compiler may complain */
-- 
2.25.1

Re: [PATCH] mm: kmalloc_index: remove case when size is more than 32MB

[Matthew Wilcox]

On Sun, May 09, 2021 at 07:13:28AM +0900, Hyeonggon Yoo wrote:
> the return value of kmalloc_index is used as index of kmalloc_caches,

it doesn't matter.  every few weeks somebody posts a patch to "optimise"
kmalloc_index, failing to appreciate that it's only ever run at compile
time because it's all under __builtin_constant_p().

Re: [PATCH] mm: kmalloc_index: remove case when size is more than 32MB

[Hyeonggon Yoo]

On Sun, May 09, 2021 at 12:19:40AM +0100, Matthew Wilcox wrote:
> On Sun, May 09, 2021 at 07:13:28AM +0900, Hyeonggon Yoo wrote:
> > the return value of kmalloc_index is used as index of kmalloc_caches,
>
> it doesn't matter.  every few weeks somebody posts a patch to "optimise"
> kmalloc_index, failing to appreciate that it's only ever run at compile
> time because it's all under __builtin_constant_p().

Oh thanks, I didn't know about __builtin_constant_p.

But I was not optimizing kmalloc_index. isn't it confusing that
kmalloc_caches alllows maximum size of 32MB, and kmalloc_index allows
maximum size of 64MB?

and even if the code I removed is never reached because 64MB is always
bigger than KMALLOC_MAX_CACHE_SIZE, it will cause an error if reached.

Re: [PATCH] mm: kmalloc_index: remove case when size is more than 32MB

[Vlastimil Babka]

On 5/9/21 7:33 AM, Hyeonggon Yoo wrote:
> On Sun, May 09, 2021 at 12:19:40AM +0100, Matthew Wilcox wrote:
>> On Sun, May 09, 2021 at 07:13:28AM +0900, Hyeonggon Yoo wrote:
>> > the return value of kmalloc_index is used as index of kmalloc_caches,
>>
>> it doesn't matter.  every few weeks somebody posts a patch to "optimise"
>> kmalloc_index, failing to appreciate that it's only ever run at compile
>> time because it's all under __builtin_constant_p().
> 
> Oh thanks, I didn't know about __builtin_constant_p.
> 
> But I was not optimizing kmalloc_index. isn't it confusing that
> kmalloc_caches alllows maximum size of 32MB, and kmalloc_index allows
> maximum size of 64MB?
> 
> and even if the code I removed is never reached because 64MB is always
> bigger than KMALLOC_MAX_CACHE_SIZE, it will cause an error if reached.

KMALLOC_MAX_CACHE_SIZE depends on KMALLOC_SHIFT_HIGH
size of kmalloc_caches array depends on KMALLOC_SHIFT_HIGH

So I don't an easy way how it could become reachable while causing the index to
overflow - if someone increased KMALLOC_SHIFT_HIGH from 25 to 26, all should be
fine, AFAICS.

The problem would be if someone increased it to 27, then we might suddenly get a
BUG() in kmalloc_index(). We should probably replace that BUG() with
BUILD_BUG_ON(1) to catch that at compile time. Hopefully no supported compiler
will break because it's not able to do the proper compile-time evaluation - but
if it does, at least we would know.

So I would accept the patch if it also changed BUG() to e.g. BUILD_BUG_ON_MSG(1,
"unexpected size in kmalloc_index()");
and expanded the function's comment that this is always compile-time evaluated
and thus no attempts at "optimizing" the code should be made.

Re: [PATCH] mm: kmalloc_index: remove case when size is more than 32MB

[Hyeonggon Yoo]

On Mon, May 10, 2021 at 12:09:55PM +0200, Vlastimil Babka wrote:
> On 5/9/21 7:33 AM, Hyeonggon Yoo wrote:
> > On Sun, May 09, 2021 at 12:19:40AM +0100, Matthew Wilcox wrote:
> >> On Sun, May 09, 2021 at 07:13:28AM +0900, Hyeonggon Yoo wrote:
> >> > the return value of kmalloc_index is used as index of kmalloc_caches,
> >>
> >> it doesn't matter.  every few weeks somebody posts a patch to "optimise"
> >> kmalloc_index, failing to appreciate that it's only ever run at compile
> >> time because it's all under __builtin_constant_p().
> > 
> > Oh thanks, I didn't know about __builtin_constant_p.
> > 
> > But I was not optimizing kmalloc_index. isn't it confusing that
> > kmalloc_caches alllows maximum size of 32MB, and kmalloc_index allows
> > maximum size of 64MB?
> > 
> > and even if the code I removed is never reached because 64MB is always
> > bigger than KMALLOC_MAX_CACHE_SIZE, it will cause an error if reached.
> 
> KMALLOC_MAX_CACHE_SIZE depends on KMALLOC_SHIFT_HIGH
> size of kmalloc_caches array depends on KMALLOC_SHIFT_HIGH
> 
> So I don't an easy way how it could become reachable while causing the index to
> overflow - if someone increased KMALLOC_SHIFT_HIGH from 25 to 26, all should be
> fine, AFAICS.
> 
> The problem would be if someone increased it to 27, then we might suddenly get a
> BUG() in kmalloc_index(). We should probably replace that BUG() with
> BUILD_BUG_ON(1) to catch that at compile time. Hopefully no supported compiler
> will break because it's not able to do the proper compile-time evaluation - but
> if it does, at least we would know.
> 
> So I would accept the patch if it also changed BUG() to e.g. BUILD_BUG_ON_MSG(1,
> "unexpected size in kmalloc_index()");
> and expanded the function's comment that this is always compile-time evaluated
> and thus no attempts at "optimizing" the code should be made.
> 

Thank you so much reviewing and replying to my patch.
plecase check if I understood well.

Okay, I'll do that work. then the following patch will:
	- remove case when size is more than 32MB
	- change "BUG to BUILD_BUG_ON to let compiler know when the size is not supported"
	- add comment that there's no need to optimize it

is it what you mean. right?

and I have a question. in the lin 751 of mm/slab_common.c,
thre's struct kmalloc_info_struct kmalloc_info. and it initializes kmalloc info
up to 64MB, which is currently not supported. should I change it too? in a separate patch?

Re: [PATCH] mm: kmalloc_index: remove case when size is more than 32MB

[Vlastimil Babka]

On 5/10/21 3:58 PM, Hyeonggon Yoo wrote:
> On Mon, May 10, 2021 at 12:09:55PM +0200, Vlastimil Babka wrote:
>> On 5/9/21 7:33 AM, Hyeonggon Yoo wrote:
>> > On Sun, May 09, 2021 at 12:19:40AM +0100, Matthew Wilcox wrote:
>> >> On Sun, May 09, 2021 at 07:13:28AM +0900, Hyeonggon Yoo wrote:
>> >> > the return value of kmalloc_index is used as index of kmalloc_caches,
>> >>
>> >> it doesn't matter.  every few weeks somebody posts a patch to "optimise"
>> >> kmalloc_index, failing to appreciate that it's only ever run at compile
>> >> time because it's all under __builtin_constant_p().
>> > 
>> > Oh thanks, I didn't know about __builtin_constant_p.
>> > 
>> > But I was not optimizing kmalloc_index. isn't it confusing that
>> > kmalloc_caches alllows maximum size of 32MB, and kmalloc_index allows
>> > maximum size of 64MB?
>> > 
>> > and even if the code I removed is never reached because 64MB is always
>> > bigger than KMALLOC_MAX_CACHE_SIZE, it will cause an error if reached.
>> 
>> KMALLOC_MAX_CACHE_SIZE depends on KMALLOC_SHIFT_HIGH
>> size of kmalloc_caches array depends on KMALLOC_SHIFT_HIGH
>> 
>> So I don't an easy way how it could become reachable while causing the index to
>> overflow - if someone increased KMALLOC_SHIFT_HIGH from 25 to 26, all should be
>> fine, AFAICS.
>> 
>> The problem would be if someone increased it to 27, then we might suddenly get a
>> BUG() in kmalloc_index(). We should probably replace that BUG() with
>> BUILD_BUG_ON(1) to catch that at compile time. Hopefully no supported compiler
>> will break because it's not able to do the proper compile-time evaluation - but
>> if it does, at least we would know.
>> 
>> So I would accept the patch if it also changed BUG() to e.g. BUILD_BUG_ON_MSG(1,
>> "unexpected size in kmalloc_index()");
>> and expanded the function's comment that this is always compile-time evaluated
>> and thus no attempts at "optimizing" the code should be made.
>> 
> 
> Thank you so much reviewing and replying to my patch.
> plecase check if I understood well.
> 
> Okay, I'll do that work. then the following patch will:
> 	- remove case when size is more than 32MB
> 	- change "BUG to BUILD_BUG_ON to let compiler know when the size is not supported"
> 	- add comment that there's no need to optimize it
> 
> is it what you mean. right?

Exactly.

> and I have a question. in the lin 751 of mm/slab_common.c,
> thre's struct kmalloc_info_struct kmalloc_info. and it initializes kmalloc info
> up to 64MB, which is currently not supported. should I change it too? in a separate patch?

Yeah that could be also changed, in the same patch.

[PATCH v2] mm: kmalloc_index: make compiler break when size is not supported

[Hyeonggon Yoo]

[-- Attachment #1: Type: text/plain, Size: 50 bytes --]

updated patch. let me know if something is wrong!

[-- Attachment #2: 0001-mm-kmalloc_index-make-compiler-break-when-size-is-no.patch --]
[-- Type: text/x-diff, Size: 2526 bytes --]

From 8fe7ecdfb0f5bd5b08771512303d72f1c6447362 Mon Sep 17 00:00:00 2001
From: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Date: Mon, 10 May 2021 23:57:34 +0900
Subject: [PATCH] mm: kmalloc_index: make compiler break when size is not
 supported

currently when size is not supported by kmalloc_index, compiler will not
break. so changed BUG to BUILD_BUG_ON_MSG to make compiler break if size is
wrong. this is done in compile time.

also removed code that allocates more than 32MB because current
implementation supports only up to 32MB.

Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 include/linux/slab.h | 7 +++++--
 mm/slab_common.c     | 7 +++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 0c97d788762c..fd0c7229d105 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -346,6 +346,9 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
  * 1 =  65 .. 96 bytes
  * 2 = 129 .. 192 bytes
  * n = 2^(n-1)+1 .. 2^n
+ *
+ * Note: you don't need to optimize kmalloc_index because it's evaluated
+ * in compile-time.
  */
 static __always_inline unsigned int kmalloc_index(size_t size)
 {
@@ -382,8 +385,8 @@ static __always_inline unsigned int kmalloc_index(size_t size)
 	if (size <=  8 * 1024 * 1024) return 23;
 	if (size <=  16 * 1024 * 1024) return 24;
 	if (size <=  32 * 1024 * 1024) return 25;
-	if (size <=  64 * 1024 * 1024) return 26;
-	BUG();
+
+	BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
 
 	/* Will never be reached. Needed because the compiler may complain */
 	return -1;
diff --git a/mm/slab_common.c b/mm/slab_common.c
index f8833d3e5d47..39d4eca8cf9b 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -745,8 +745,8 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
 
 /*
  * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time.
- * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is
- * kmalloc-67108864.
+ * kmalloc_index() supports up to 2^25=32MB, so the final entry of the table is
+ * kmalloc-33554432.
  */
 const struct kmalloc_info_struct kmalloc_info[] __initconst = {
 	INIT_KMALLOC_INFO(0, 0),
@@ -774,8 +774,7 @@ const struct kmalloc_info_struct kmalloc_info[] __initconst = {
 	INIT_KMALLOC_INFO(4194304, 4M),
 	INIT_KMALLOC_INFO(8388608, 8M),
 	INIT_KMALLOC_INFO(16777216, 16M),
-	INIT_KMALLOC_INFO(33554432, 32M),
-	INIT_KMALLOC_INFO(67108864, 64M)
+	INIT_KMALLOC_INFO(33554432, 32M)
 };
 
 /*
-- 
2.25.1

Re: [PATCH v2] mm: kmalloc_index: make compiler break when size is not supported

[Christoph Lameter]

[-- Attachment #1: Type: text/plain, Size: 235 bytes --]

I guess this needs to be reviewed and tested by the users of architectures
that can use large MAXORDER pages such as powerpc and Itanium.

On Tue, 11 May 2021, Hyeonggon Yoo wrote:

> updated patch. let me know if something is wrong!
>

[-- Attachment #2: Type: text/x-diff, Size: 2527 bytes --]

>From 8fe7ecdfb0f5bd5b08771512303d72f1c6447362 Mon Sep 17 00:00:00 2001
From: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Date: Mon, 10 May 2021 23:57:34 +0900
Subject: [PATCH] mm: kmalloc_index: make compiler break when size is not
 supported

currently when size is not supported by kmalloc_index, compiler will not
break. so changed BUG to BUILD_BUG_ON_MSG to make compiler break if size is
wrong. this is done in compile time.

also removed code that allocates more than 32MB because current
implementation supports only up to 32MB.

Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 include/linux/slab.h | 7 +++++--
 mm/slab_common.c     | 7 +++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 0c97d788762c..fd0c7229d105 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -346,6 +346,9 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
  * 1 =  65 .. 96 bytes
  * 2 = 129 .. 192 bytes
  * n = 2^(n-1)+1 .. 2^n
+ *
+ * Note: you don't need to optimize kmalloc_index because it's evaluated
+ * in compile-time.
  */
 static __always_inline unsigned int kmalloc_index(size_t size)
 {
@@ -382,8 +385,8 @@ static __always_inline unsigned int kmalloc_index(size_t size)
 	if (size <=  8 * 1024 * 1024) return 23;
 	if (size <=  16 * 1024 * 1024) return 24;
 	if (size <=  32 * 1024 * 1024) return 25;
-	if (size <=  64 * 1024 * 1024) return 26;
-	BUG();
+
+	BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
 
 	/* Will never be reached. Needed because the compiler may complain */
 	return -1;
diff --git a/mm/slab_common.c b/mm/slab_common.c
index f8833d3e5d47..39d4eca8cf9b 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -745,8 +745,8 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
 
 /*
  * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time.
- * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is
- * kmalloc-67108864.
+ * kmalloc_index() supports up to 2^25=32MB, so the final entry of the table is
+ * kmalloc-33554432.
  */
 const struct kmalloc_info_struct kmalloc_info[] __initconst = {
 	INIT_KMALLOC_INFO(0, 0),
@@ -774,8 +774,7 @@ const struct kmalloc_info_struct kmalloc_info[] __initconst = {
 	INIT_KMALLOC_INFO(4194304, 4M),
 	INIT_KMALLOC_INFO(8388608, 8M),
 	INIT_KMALLOC_INFO(16777216, 16M),
-	INIT_KMALLOC_INFO(33554432, 32M),
-	INIT_KMALLOC_INFO(67108864, 64M)
+	INIT_KMALLOC_INFO(33554432, 32M)
 };
 
 /*
-- 
2.25.1

Re: [PATCH v2] mm: kmalloc_index: make compiler break when size is not supported

[Vlastimil Babka]

On 5/10/21 5:02 PM, Hyeonggon Yoo wrote:
> updated patch. let me know if something is wrong!
> 
> 
> 0001-mm-kmalloc_index-make-compiler-break-when-size-is-no.patch
> 
> From 8fe7ecdfb0f5bd5b08771512303d72f1c6447362 Mon Sep 17 00:00:00 2001
> From: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> Date: Mon, 10 May 2021 23:57:34 +0900
> Subject: [PATCH] mm: kmalloc_index: make compiler break when size is not
>  supported

I'd rephrase the subject:
mm, slub: change run-time assertion in kmalloc_index() to compile-time

> currently when size is not supported by kmalloc_index, compiler will not
> break.

"... compiler will generate a run-time BUG() while a compile-time error is also
possible, and better"

> so changed BUG to BUILD_BUG_ON_MSG to make compiler break if size is
> wrong. this is done in compile time.
> 
> also removed code that allocates more than 32MB because current
> implementation supports only up to 32MB.
> 
> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> ---
>  include/linux/slab.h | 7 +++++--
>  mm/slab_common.c     | 7 +++----
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 0c97d788762c..fd0c7229d105 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -346,6 +346,9 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
>   * 1 =  65 .. 96 bytes
>   * 2 = 129 .. 192 bytes
>   * n = 2^(n-1)+1 .. 2^n
> + *
> + * Note: you don't need to optimize kmalloc_index because it's evaluated

"there's no need to..."

> + * in compile-time.
>   */
>  static __always_inline unsigned int kmalloc_index(size_t size)
>  {
> @@ -382,8 +385,8 @@ static __always_inline unsigned int kmalloc_index(size_t size)
>  	if (size <=  8 * 1024 * 1024) return 23;
>  	if (size <=  16 * 1024 * 1024) return 24;
>  	if (size <=  32 * 1024 * 1024) return 25;
> -	if (size <=  64 * 1024 * 1024) return 26;
> -	BUG();
> +
> +	BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
>  
>  	/* Will never be reached. Needed because the compiler may complain */
>  	return -1;
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index f8833d3e5d47..39d4eca8cf9b 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -745,8 +745,8 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
>  
>  /*
>   * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time.
> - * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is
> - * kmalloc-67108864.
> + * kmalloc_index() supports up to 2^25=32MB, so the final entry of the table is
> + * kmalloc-33554432.

      kmalloc-32M

>   */
>  const struct kmalloc_info_struct kmalloc_info[] __initconst = {
>  	INIT_KMALLOC_INFO(0, 0),
> @@ -774,8 +774,7 @@ const struct kmalloc_info_struct kmalloc_info[] __initconst = {
>  	INIT_KMALLOC_INFO(4194304, 4M),
>  	INIT_KMALLOC_INFO(8388608, 8M),
>  	INIT_KMALLOC_INFO(16777216, 16M),
> -	INIT_KMALLOC_INFO(33554432, 32M),
> -	INIT_KMALLOC_INFO(67108864, 64M)
> +	INIT_KMALLOC_INFO(33554432, 32M)
>  };
>  
>  /*
> 

Thanks

Re: [PATCH v2] mm: kmalloc_index: make compiler break when size is not supported

[Vlastimil Babka]

On 5/10/21 5:15 PM, Christoph Lameter wrote:
> I guess this needs to be reviewed and tested by the users of architectures
> that can use large MAXORDER pages such as powerpc and Itanium.

AFAICS large MAX_ORDER should matter as KMALLOC_SHIFT_HIGH will be always capped
to 25. But sure, let the bots complain if something is wrong. I'm more
interested if we shake out some compiler that can't do the compile-time
evaluation properly and we didn't know until now.

> On Tue, 11 May 2021, Hyeonggon Yoo wrote:
> 
>> updated patch. let me know if something is wrong!
>>
> 
> 
> 0001-mm-kmalloc_index-make-compiler-break-when-size-is-no.patch
> 
>>From 8fe7ecdfb0f5bd5b08771512303d72f1c6447362 Mon Sep 17 00:00:00 2001
> From: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> Date: Mon, 10 May 2021 23:57:34 +0900
> Subject: [PATCH] mm: kmalloc_index: make compiler break when size is not
>  supported
> 
> currently when size is not supported by kmalloc_index, compiler will not
> break. so changed BUG to BUILD_BUG_ON_MSG to make compiler break if size is
> wrong. this is done in compile time.
> 
> also removed code that allocates more than 32MB because current
> implementation supports only up to 32MB.
> 
> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> ---
>  include/linux/slab.h | 7 +++++--
>  mm/slab_common.c     | 7 +++----
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 0c97d788762c..fd0c7229d105 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -346,6 +346,9 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
>   * 1 =  65 .. 96 bytes
>   * 2 = 129 .. 192 bytes
>   * n = 2^(n-1)+1 .. 2^n
> + *
> + * Note: you don't need to optimize kmalloc_index because it's evaluated
> + * in compile-time.
>   */
>  static __always_inline unsigned int kmalloc_index(size_t size)
>  {
> @@ -382,8 +385,8 @@ static __always_inline unsigned int kmalloc_index(size_t size)
>  	if (size <=  8 * 1024 * 1024) return 23;
>  	if (size <=  16 * 1024 * 1024) return 24;
>  	if (size <=  32 * 1024 * 1024) return 25;
> -	if (size <=  64 * 1024 * 1024) return 26;
> -	BUG();
> +
> +	BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
>  
>  	/* Will never be reached. Needed because the compiler may complain */
>  	return -1;
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index f8833d3e5d47..39d4eca8cf9b 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -745,8 +745,8 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
>  
>  /*
>   * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time.
> - * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is
> - * kmalloc-67108864.
> + * kmalloc_index() supports up to 2^25=32MB, so the final entry of the table is
> + * kmalloc-33554432.
>   */
>  const struct kmalloc_info_struct kmalloc_info[] __initconst = {
>  	INIT_KMALLOC_INFO(0, 0),
> @@ -774,8 +774,7 @@ const struct kmalloc_info_struct kmalloc_info[] __initconst = {
>  	INIT_KMALLOC_INFO(4194304, 4M),
>  	INIT_KMALLOC_INFO(8388608, 8M),
>  	INIT_KMALLOC_INFO(16777216, 16M),
> -	INIT_KMALLOC_INFO(33554432, 32M),
> -	INIT_KMALLOC_INFO(67108864, 64M)
> +	INIT_KMALLOC_INFO(33554432, 32M)
>  };
>  
>  /*
> 

Re: [PATCH v2] mm: kmalloc_index: make compiler break when size is not supported

[Hyeonggon Yoo]

On Mon, May 10, 2021 at 05:19:58PM +0200, Vlastimil Babka wrote:

> I'd rephrase the subject:
> mm, slub: change run-time assertion in kmalloc_index() to compile-time
>

> "... compiler will generate a run-time BUG() while a compile-time error is also
> possible, and better"

> "there's no need to..."
>       kmalloc-32M


Vlastimil Babka and Christoph Lameter, thank you for reviewing the patch.

I'm not familiar with kernel community yet. should I send patch v3 again,
or can you update it directly?

Re: [PATCH v2] mm: kmalloc_index: make compiler break when size is not supported

[Matthew Wilcox]

On Tue, May 11, 2021 at 12:02:30AM +0900, Hyeonggon Yoo wrote:
> @@ -382,8 +385,8 @@ static __always_inline unsigned int kmalloc_index(size_t size)
>  	if (size <=  8 * 1024 * 1024) return 23;
>  	if (size <=  16 * 1024 * 1024) return 24;
>  	if (size <=  32 * 1024 * 1024) return 25;
> -	if (size <=  64 * 1024 * 1024) return 26;
> -	BUG();
> +
> +	BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");

we're being encouraged to use static_assert() these days.
https://en.cppreference.com/w/c/language/_Static_assert

Re: [PATCH v3] mm: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

From c2b8fc4144f2c44b0ac5957a06207c385cc94d15 Mon Sep 17 00:00:00 2001
From: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Date: Tue, 11 May 2021 11:53:23 +0900
Subject: [PATCH] mm, slub: change run-time assertion in kmalloc_index() to
 compile-time

currently when size is not supported by kmalloc_index, compiler will
generate a run-time BUG() while compile-time error is also possible,
and better. so changed BUG to BUILD_BUG_ON_MSG to make compile-time
check possible.

also removed code that allocates more than 32MB because current
implementation supports only up to 32MB.

Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 include/linux/slab.h | 7 +++++--
 mm/slab_common.c     | 7 +++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 6d454886bcaf..df1937309df2 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -345,6 +345,9 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
  * 1 =  65 .. 96 bytes
  * 2 = 129 .. 192 bytes
  * n = 2^(n-1)+1 .. 2^n
+ *
+ * Note: there's no need to optimize kmalloc_index because it's evaluated
+ * in compile-time.
  */
 static __always_inline unsigned int kmalloc_index(size_t size)
 {
@@ -381,8 +384,8 @@ static __always_inline unsigned int kmalloc_index(size_t size)
 	if (size <=  8 * 1024 * 1024) return 23;
 	if (size <=  16 * 1024 * 1024) return 24;
 	if (size <=  32 * 1024 * 1024) return 25;
-	if (size <=  64 * 1024 * 1024) return 26;
-	BUG();
+
+	BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
 
 	/* Will never be reached. Needed because the compiler may complain */
 	return -1;
diff --git a/mm/slab_common.c b/mm/slab_common.c
index fe8b68482670..97664bbe8147 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -1192,8 +1192,8 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
 
 /*
  * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time.
- * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is
- * kmalloc-67108864.
+ * kmalloc_index() supports up to 2^25=32MB, so the final entry of the table is
+ * kmalloc-32M.
  */
 const struct kmalloc_info_struct kmalloc_info[] __initconst = {
 	INIT_KMALLOC_INFO(0, 0),
@@ -1221,8 +1221,7 @@ const struct kmalloc_info_struct kmalloc_info[] __initconst = {
 	INIT_KMALLOC_INFO(4194304, 4M),
 	INIT_KMALLOC_INFO(8388608, 8M),
 	INIT_KMALLOC_INFO(16777216, 16M),
-	INIT_KMALLOC_INFO(33554432, 32M),
-	INIT_KMALLOC_INFO(67108864, 64M)
+	INIT_KMALLOC_INFO(33554432, 32M)
 };
 
 /*
-- 
2.25.1

Re: [PATCH v2] mm: kmalloc_index: make compiler break when size is not supported

[Hyeonggon Yoo]

On Mon, May 10, 2021 at 04:44:09PM +0100, Matthew Wilcox wrote:
> On Tue, May 11, 2021 at 12:02:30AM +0900, Hyeonggon Yoo wrote:
> > @@ -382,8 +385,8 @@ static __always_inline unsigned int kmalloc_index(size_t size)
> >  	if (size <=  8 * 1024 * 1024) return 23;
> >  	if (size <=  16 * 1024 * 1024) return 24;
> >  	if (size <=  32 * 1024 * 1024) return 25;
> > -	if (size <=  64 * 1024 * 1024) return 26;
> > -	BUG();
> > +
> > +	BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
> 
> we're being encouraged to use static_assert() these days.
> https://en.cppreference.com/w/c/language/_Static_assert
> 

can you tell me difference between static_assert and BUILD_BUG_ON?
it seems that mm subsystem does not make use of it now.

Re: [PATCH v2] mm: kmalloc_index: make compiler break when size is not supported

[Hyeonggon Yoo]

On Mon, May 10, 2021 at 05:15:23PM +0200, Christoph Lameter wrote:
> I guess this needs to be reviewed and tested by the users of architectures
> that can use large MAXORDER pages such as powerpc and Itanium.

as Vlastimil Babka said, I think it's fine too. because
KMALLOC_SHIFT_HIGH can't be bigger than 25 regardless of MAX_ORDER

Re: [PATCH v2] mm: kmalloc_index: make compiler break when size is not supported

[Vlastimil Babka]

On 5/11/21 5:03 AM, Hyeonggon Yoo wrote:
> On Mon, May 10, 2021 at 04:44:09PM +0100, Matthew Wilcox wrote:
>> On Tue, May 11, 2021 at 12:02:30AM +0900, Hyeonggon Yoo wrote:
>> > @@ -382,8 +385,8 @@ static __always_inline unsigned int kmalloc_index(size_t size)
>> >  	if (size <=  8 * 1024 * 1024) return 23;
>> >  	if (size <=  16 * 1024 * 1024) return 24;
>> >  	if (size <=  32 * 1024 * 1024) return 25;
>> > -	if (size <=  64 * 1024 * 1024) return 26;
>> > -	BUG();
>> > +
>> > +	BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
>> 
>> we're being encouraged to use static_assert() these days.
>> https://en.cppreference.com/w/c/language/_Static_assert
>> 
> 
> can you tell me difference between static_assert and BUILD_BUG_ON?
> it seems that mm subsystem does not make use of it now.

Some difference is explained in include/linux/build_bug.h near static_assert
definition.
But importantly it seems it's not possible to place static_assert(false) in
place of the BUG() because it will trigger despite not being reachable.
BUILD_BUG_ON_MSG(1  "..."); worked as expected for me.

Re: [PATCH v2] mm: kmalloc_index: make compiler break when size is not supported

[Vlastimil Babka]

On 5/10/21 5:38 PM, Hyeonggon Yoo wrote:
> On Mon, May 10, 2021 at 05:19:58PM +0200, Vlastimil Babka wrote:
> 
>> I'd rephrase the subject:
>> mm, slub: change run-time assertion in kmalloc_index() to compile-time
>>
> 
>> "... compiler will generate a run-time BUG() while a compile-time error is also
>> possible, and better"
> 
>> "there's no need to..."
>>       kmalloc-32M
> 
> 
> Vlastimil Babka and Christoph Lameter, thank you for reviewing the patch.
> 
> I'm not familiar with kernel community yet. should I send patch v3 again,
> or can you update it directly?

I think it would be best if you sent v3, the way you did with v1 - inline. With
v2 it looked like a mail body with you message and patch as attachment. We want
the testing bots to pick it up and they might work only with inline patch. Thanks.

Re: [PATCH v2] mm: kmalloc_index: make compiler break when size is not supported

[Vlastimil Babka]

On 5/11/21 10:36 AM, Vlastimil Babka wrote:
> On 5/10/21 5:38 PM, Hyeonggon Yoo wrote:
>> On Mon, May 10, 2021 at 05:19:58PM +0200, Vlastimil Babka wrote:
>> 
>>> I'd rephrase the subject:
>>> mm, slub: change run-time assertion in kmalloc_index() to compile-time
>>>
>> 
>>> "... compiler will generate a run-time BUG() while a compile-time error is also
>>> possible, and better"
>> 
>>> "there's no need to..."
>>>       kmalloc-32M
>> 
>> 
>> Vlastimil Babka and Christoph Lameter, thank you for reviewing the patch.
>> 
>> I'm not familiar with kernel community yet. should I send patch v3 again,
>> or can you update it directly?
> 
> I think it would be best if you sent v3, the way you did with v1 - inline. With
> v2 it looked like a mail body with you message and patch as attachment. We want
> the testing bots to pick it up and they might work only with inline patch. Thanks.

Ah, you already did. Good.

Re: [PATCH v2] mm: kmalloc_index: make compiler break when size is not supported

[Hyeonggon Yoo]

On Tue, May 11, 2021 at 10:37:20AM +0200, Vlastimil Babka wrote:
> >
> > I think it would be best if you sent v3, the way you did with v1 - inline. With
> > v2 it looked like a mail body with you message and patch as attachment. We want
> > the testing bots to pick it up and they might work only with inline patch. Thanks.
>
> Ah, you already did. Good.
>

OK, I will not send patch as an attachment later. it's first time for me
to hear about 'testing bots'. is there a documentation about it?