mm/slab: Avoid build bug for calls to kmalloc with a large constant

[RFC PATCH] mm/slab: Avoid build bug for calls to kmalloc with a large constant

Posted by Dave Kleikamp 3 weeks, 1 day ago

When boot-time page size is enabled, the test against KMALLOC_MAX_CACHE_SIZE
is no longer optimized out with a constant size, so a build bug may
occur on a path that won't be reached.

Found compiling drivers/net/ethernet/qlogic/qed/qed_sriov.c

Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>
---

Ryan,

Please consider incorporating this fix or something similar into your
mm patch in the boot-time pages size patches.

  include/linux/slab.h | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 9848296ca6ba..a4c7507ab8ec 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -685,7 +685,8 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
  	if (size <= 1024 * 1024) return 20;
  	if (size <=  2 * 1024 * 1024) return 21;
  
-	if (!IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) && size_is_constant)
+	if (!IS_ENABLED(CONFIG_ARM64_BOOT_TIME_PAGE_SIZE) &&
+	    !IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) && size_is_constant)
  		BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
  	else
  		BUG();
-- 
2.47.0

Re: [RFC PATCH] mm/slab: Avoid build bug for calls to kmalloc with a large constant

Posted by Vlastimil Babka 1 week, 3 days ago

On 11/1/24 21:16, Dave Kleikamp wrote:
> When boot-time page size is enabled, the test against KMALLOC_MAX_CACHE_SIZE
> is no longer optimized out with a constant size, so a build bug may
> occur on a path that won't be reached.

That's rather unfortunate, the __builtin_constant_p(size) part of
kmalloc_noprof() really expects things to resolve at compile time and it
would be better to keep it that way.

I think it would be better if we based KMALLOC_MAX_CACHE_SIZE itself on
PAGE_SHIFT_MAX and kept it constant, instead of introducing
KMALLOC_SHIFT_HIGH_MAX only for some sanity checks.

So if the kernel was built to support 4k to 64k, but booted as 4k, it would
still create and use kmalloc caches up to 128k. SLUB should handle that fine
(if not, please report it :)

Maybe we could also stop adding + 1 to PAGE_SHIFT_MAX if it's >=64k, so the
cache size is max 64k and not 128k but that should be probably evaluated
separately from this series.

Vlastimil

> Found compiling drivers/net/ethernet/qlogic/qed/qed_sriov.c
> 
> Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>
> ---
> 
> Ryan,
> 
> Please consider incorporating this fix or something similar into your
> mm patch in the boot-time pages size patches.
> 
>   include/linux/slab.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 9848296ca6ba..a4c7507ab8ec 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -685,7 +685,8 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
>   	if (size <= 1024 * 1024) return 20;
>   	if (size <=  2 * 1024 * 1024) return 21;
>   
> -	if (!IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) && size_is_constant)
> +	if (!IS_ENABLED(CONFIG_ARM64_BOOT_TIME_PAGE_SIZE) &&
> +	    !IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) && size_is_constant)
>   		BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
>   	else
>   		BUG();

Re: [RFC PATCH] mm/slab: Avoid build bug for calls to kmalloc with a large constant

Posted by Ryan Roberts 2 weeks, 4 days ago

On 01/11/2024 20:16, Dave Kleikamp wrote:
> When boot-time page size is enabled, the test against KMALLOC_MAX_CACHE_SIZE
> is no longer optimized out with a constant size, so a build bug may
> occur on a path that won't be reached.
> 
> Found compiling drivers/net/ethernet/qlogic/qed/qed_sriov.c
> 
> Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>
> ---
> 
> Ryan,
> 
> Please consider incorporating this fix or something similar into your
> mm patch in the boot-time pages size patches.
> 
>  include/linux/slab.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 9848296ca6ba..a4c7507ab8ec 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -685,7 +685,8 @@ static __always_inline unsigned int __kmalloc_index(size_t
> size,
>      if (size <= 1024 * 1024) return 20;
>      if (size <=  2 * 1024 * 1024) return 21;
>  
> -    if (!IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) && size_is_constant)
> +    if (!IS_ENABLED(CONFIG_ARM64_BOOT_TIME_PAGE_SIZE) &&

Thanks for the patch! I think this may be better as:

       if (PAGE_SHIFT_MIN == PAGE_SHIFT_MAX &&

Since that is independent of the architecture. Your approach wouldn't work if
another arch wanted to enable boot time page size, or if arm64 dropped the
Kconfig because it decided only boot time page size will be supported in future.

Thanks,
Ryan

> +        !IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) && size_is_constant)
>          BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
>      else
>          BUG();

Re: [RFC PATCH] mm/slab: Avoid build bug for calls to kmalloc with a large constant

Posted by Dave Kleikamp 2 weeks, 4 days ago

On 11/6/24 5:44AM, Ryan Roberts wrote:
> On 01/11/2024 20:16, Dave Kleikamp wrote:
>> When boot-time page size is enabled, the test against KMALLOC_MAX_CACHE_SIZE
>> is no longer optimized out with a constant size, so a build bug may
>> occur on a path that won't be reached.
>>
>> Found compiling drivers/net/ethernet/qlogic/qed/qed_sriov.c
>>
>> Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>
>> ---
>>
>> Ryan,
>>
>> Please consider incorporating this fix or something similar into your
>> mm patch in the boot-time pages size patches.
>>
>>   include/linux/slab.h | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>> index 9848296ca6ba..a4c7507ab8ec 100644
>> --- a/include/linux/slab.h
>> +++ b/include/linux/slab.h
>> @@ -685,7 +685,8 @@ static __always_inline unsigned int __kmalloc_index(size_t
>> size,
>>       if (size <= 1024 * 1024) return 20;
>>       if (size <=  2 * 1024 * 1024) return 21;
>>   
>> -    if (!IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) && size_is_constant)
>> +    if (!IS_ENABLED(CONFIG_ARM64_BOOT_TIME_PAGE_SIZE) &&
> 
> Thanks for the patch! I think this may be better as:
> 
>         if (PAGE_SHIFT_MIN == PAGE_SHIFT_MAX &&
> 
> Since that is independent of the architecture. Your approach wouldn't work if
> another arch wanted to enable boot time page size, or if arm64 dropped the
> Kconfig because it decided only boot time page size will be supported in future.

Absolutely. I may be sending some more. I haven't gotten to JFS yet, but 
that one is my responsibility.

Thanks,
Shaggy

> 
> Thanks,
> Ryan
> 
>> +        !IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) && size_is_constant)
>>           BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
>>       else
>>           BUG();
>