Re: [PATCH] mm/slab: call __kmalloc_noprof() when size isn't builtin constant in kmalloc_array_noprof()

[Harry Yoo <harry.yoo@oracle.com>]

On Tue, Mar 04, 2025 at 11:03:54AM +0100, Vlastimil Babka wrote:
> On 3/2/25 14:30, Harry Yoo wrote:
> > On Sat, Mar 01, 2025 at 04:37:20PM +0800, Ye Bin wrote:
> >> From: Ye Bin <yebin10@huawei.com>
> >> 
> >> If 'n' or 'size' isn't builtin constant just call __kmalloc_noprof()
> >> in kmalloc_array_noprof().
> >> 
> >> Fixes: 7bd230a26648 ("mm/slab: enable slab allocation tagging for kmalloc and friends")
> >> Signed-off-by: Ye Bin <yebin10@huawei.com>
> >> ---
> >>  include/linux/slab.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/include/linux/slab.h b/include/linux/slab.h
> >> index 3b03b31831a9..0edb8363fc4f 100644
> >> --- a/include/linux/slab.h
> >> +++ b/include/linux/slab.h
> >> @@ -947,7 +947,7 @@ static inline __alloc_size(1, 2) void *kmalloc_array_noprof(size_t n, size_t siz
> >>  		return NULL;
> >>  	if (__builtin_constant_p(n) && __builtin_constant_p(size))
> >>  		return kmalloc_noprof(bytes, flags);
> >> -	return kmalloc_noprof(bytes, flags);
> >> +	return __kmalloc_noprof(bytes, flags);
> > 
> > Can we simply call kmalloc_noprof(bytes, flags) once instead?
> > 
> > The compiler should know 'bytes' is a constant if both 'n' and 'size' are
> > constants.
> 
> Right, applied like this, hope it's ok?
> 
> 
> commit a6553ee4b00ba4b1466a81fb00fa4679b7ce5339 (HEAD -> slab/for-6.15/fixes-cleanups)
> Author: Ye Bin <yebin10@huawei.com>
> Date:   Sat Mar 1 16:37:20 2025 +0800
> 
>     mm/slab: call kmalloc_noprof() unconditionally in kmalloc_array_noprof()
>     
>     If 'n' or 'size' isn't builtin constant, we used to call __kmalloc()
>     before commit 7bd230a26648 ("mm/slab: enable slab allocation tagging for
>     kmalloc and friends"), which inadvertedly changed both paths to
>     kmalloc_noprof().
>     
>     As Harry Yoo points out we can just call kmalloc_noprof()
>     unconditionally. If the compiler knows n and size are constants it
>     doesn't guarantee that bytes will be also seen as constant, and that is
>     the important test in kmalloc_noprof() here, so we can just defer to it
>     always.
>     
>     [ vbabka@suse.cz: change as Harry suggested and adjust commit log ]
>     
>     Fixes: 7bd230a26648 ("mm/slab: enable slab allocation tagging for kmalloc and friends")
>     Signed-off-by: Ye Bin <yebin10@huawei.com>
>     Signed-off-by: Vlastimil Babka <vbabka@suse.cz>


Looks good to me, thanks!

FWIW,
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

-- 
Cheers,
Harry

> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 09eedaecf120..ab05a143d09a 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -941,8 +941,6 @@ static inline __alloc_size(1, 2) void *kmalloc_array_noprof(size_t n, size_t siz
>  
>         if (unlikely(check_mul_overflow(n, size, &bytes)))
>                 return NULL;
> -       if (__builtin_constant_p(n) && __builtin_constant_p(size))
> -               return kmalloc_noprof(bytes, flags);
>         return kmalloc_noprof(bytes, flags);
>  }
>  #define kmalloc_array(...)                     alloc_hooks(kmalloc_array_noprof(__VA_ARGS__))
>