diff mbox series

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

Message ID 20210510150230.GA74915@hyeyoo (mailing list archive)
State New
Headers show
Series [v2] mm: kmalloc_index: make compiler break when size is not supported | expand

Commit Message

Hyeonggon Yoo May 10, 2021, 3:02 p.m. UTC
updated patch. let me know if something is wrong!

Comments

Christoph Lameter May 10, 2021, 3:15 p.m. UTC | #1
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!
>
Vlastimil Babka May 10, 2021, 3:19 p.m. UTC | #2
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
Vlastimil Babka May 10, 2021, 3:21 p.m. UTC | #3
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)
>  };
>  
>  /*
>
Hyeonggon Yoo May 10, 2021, 3:38 p.m. UTC | #4
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?
Matthew Wilcox May 10, 2021, 3:44 p.m. UTC | #5
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
Hyeonggon Yoo May 11, 2021, 2:59 a.m. UTC | #6
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)
 };
 
 /*
Hyeonggon Yoo May 11, 2021, 3:03 a.m. UTC | #7
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.
Hyeonggon Yoo May 11, 2021, 3:09 a.m. UTC | #8
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
Vlastimil Babka May 11, 2021, 8:33 a.m. UTC | #9
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.
Vlastimil Babka May 11, 2021, 8:36 a.m. UTC | #10
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.
Vlastimil Babka May 11, 2021, 8:37 a.m. UTC | #11
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.
Hyeonggon Yoo May 11, 2021, 9:14 a.m. UTC | #12
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?
diff mbox series

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)
 };
 
 /*
-- 
2.25.1