| Message ID | 20210510150230.GA74915@hyeyoo (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm: kmalloc_index: make compiler break when size is not supported | expand |
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! >
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
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) > }; > > /* >
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?
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
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) }; /*
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.
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
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.
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.
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.
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?
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