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

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

[Hyeonggon Yoo]

Hello Vlastimil, recently kbuild-all test bot reported compile error on
clang 10.0.1, with defconfig.

Nathan Chancellor wrote:
> I think this happens because arch_prepare_optimized_kprobe() calls kzalloc()
> with a size of MAX_OPTINSN_SIZE, which is
> 
> #define MAX_OPTINSN_SIZE                                \
>       (((unsigned long)optprobe_template_end -        \
>          (unsigned long)optprobe_template_entry) +     \
>         MAX_OPTIMIZED_LENGTH + JMP32_INSN_SIZE)

> and the optprobe_template_{end,entry} are not evaluated as constants.
>
> I am not sure what the solution is. There seem to be a growing list of issues
> with LLVM 10 that were fixed in LLVM 11, which might necessitate requiring
> LLVM 11 and newer to build the kernel, given this affects a defconfig.
> Cheers,
> Nathan


I think it's because kmalloc compiles successfully when size is constant,
and kmalloc_index isn't. so I think compiler seems to be confused.

currently if size is non-constant, kmalloc calls dummy function __kmalloc,
which always returns NULL.

so what about changing kmalloc to do compile-time assertion too, and track
all callers that are calling kmalloc with non-constant argument.

How do you think? If you think it is the solution, I'll do that work.

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

[Vlastimil Babka]

On 5/15/21 11:09 PM, Hyeonggon Yoo wrote:
> Hello Vlastimil, recently kbuild-all test bot reported compile error on
> clang 10.0.1, with defconfig.

Hm yes, catching some compiler bug was something that was noted to be
possible to happen.

> Nathan Chancellor wrote:
>> I think this happens because arch_prepare_optimized_kprobe() calls kzalloc()
>> with a size of MAX_OPTINSN_SIZE, which is
>>
>> #define MAX_OPTINSN_SIZE                                \
>>       (((unsigned long)optprobe_template_end -        \
>>          (unsigned long)optprobe_template_entry) +     \
>>         MAX_OPTIMIZED_LENGTH + JMP32_INSN_SIZE)
> 
>> and the optprobe_template_{end,entry} are not evaluated as constants.
>>
>> I am not sure what the solution is. There seem to be a growing list of issues
>> with LLVM 10 that were fixed in LLVM 11, which might necessitate requiring
>> LLVM 11 and newer to build the kernel, given this affects a defconfig.
>> Cheers,
>> Nathan
> 
> 
> I think it's because kmalloc compiles successfully when size is constant,
> and kmalloc_index isn't. so I think compiler seems to be confused.
> 
> currently if size is non-constant, kmalloc calls dummy function __kmalloc,
> which always returns NULL.

That's a misunderstanding. __kmalloc() is not a dummy function, you
probably found only the header declaration.

> so what about changing kmalloc to do compile-time assertion too, and track
> all callers that are calling kmalloc with non-constant argument.

kmalloc() is expected to be called with both constant and non-constant
size. __builtin_constant_p() is used to determine which implementation
to use. One based on kmalloc_index(), other on __kmalloc().

It appears clang 10.0.1 is mistakenly evaluating __builtin_constant_p()
as true. Probably something to do with LTO, because MAX_OPTINSN_SIZE
seems it could be a "link-time constant".

Maybe we could extend Marco Elver's followup patch that uses
BUILD_BUG_ON vs BUG() depending on size_is_constant parameter. It could
use BUG() also if the compiler is LLVM < 11 or something. What would be
the proper code for this condition?

> How do you think? If you think it is the solution, I'll do that work.
> 

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

[Hyeonggon Yoo]

On Sat, May 15, 2021 at 11:24:25PM +0200, Vlastimil Babka wrote:
>
> That's a misunderstanding. __kmalloc() is not a dummy function, you
> probably found only the header declaration.
>

Sorry, that was totally my misunderstanding.
I was reading dummy function in arch/alpha/boot/bootpz.c:415.
I wrongly configured the tool.

> It appears clang 10.0.1 is mistakenly evaluating __builtin_constant_p()
> as true. Probably something to do with LTO, because MAX_OPTINSN_SIZE
> seems it could be a "link-time constant".

That is what I was missing. Thank you for kindly explaining it.

> Maybe we could extend Marco Elver's followup patch that uses
> BUILD_BUG_ON vs BUG() depending on size_is_constant parameter. It could
> use BUG() also if the compiler is LLVM < 11 or something. What would be
> the proper code for this condition?

Fixing clang's bug in linux kernel doesn't seem to be a solution.
So now I understand why Nathan said we might require LLVM > 11.

I thought I should do something to fix it because I sent the patch.
but I was misunderstanding a lot. Thank you sincerely for letting me know.

Thanks,

Hyeonggon

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

[Nathan Chancellor]

On Sat, May 15, 2021 at 11:24:25PM +0200, Vlastimil Babka wrote:
> On 5/15/21 11:09 PM, Hyeonggon Yoo wrote:
> > Hello Vlastimil, recently kbuild-all test bot reported compile error on
> > clang 10.0.1, with defconfig.
> 
> Hm yes, catching some compiler bug was something that was noted to be
> possible to happen.
> 
> > Nathan Chancellor wrote:
> >> I think this happens because arch_prepare_optimized_kprobe() calls kzalloc()
> >> with a size of MAX_OPTINSN_SIZE, which is
> >>
> >> #define MAX_OPTINSN_SIZE                                \
> >>       (((unsigned long)optprobe_template_end -        \
> >>          (unsigned long)optprobe_template_entry) +     \
> >>         MAX_OPTIMIZED_LENGTH + JMP32_INSN_SIZE)
> > 
> >> and the optprobe_template_{end,entry} are not evaluated as constants.
> >>
> >> I am not sure what the solution is. There seem to be a growing list of issues
> >> with LLVM 10 that were fixed in LLVM 11, which might necessitate requiring
> >> LLVM 11 and newer to build the kernel, given this affects a defconfig.
> >> Cheers,
> >> Nathan
> > 
> > 
> > I think it's because kmalloc compiles successfully when size is constant,
> > and kmalloc_index isn't. so I think compiler seems to be confused.
> > 
> > currently if size is non-constant, kmalloc calls dummy function __kmalloc,
> > which always returns NULL.
> 
> That's a misunderstanding. __kmalloc() is not a dummy function, you
> probably found only the header declaration.
> 
> > so what about changing kmalloc to do compile-time assertion too, and track
> > all callers that are calling kmalloc with non-constant argument.
> 
> kmalloc() is expected to be called with both constant and non-constant
> size. __builtin_constant_p() is used to determine which implementation
> to use. One based on kmalloc_index(), other on __kmalloc().
> 
> It appears clang 10.0.1 is mistakenly evaluating __builtin_constant_p()
> as true. Probably something to do with LTO, because MAX_OPTINSN_SIZE
> seems it could be a "link-time constant".

This happens with x86_64 defconfig so LTO is not involved.

However, the explanation makes sense, given that the LLVM change I
landed on changes the sparse conditional constant propagation pass,
which I believe can influence how LLVM handles __builtin_constant_p().

> Maybe we could extend Marco Elver's followup patch that uses
> BUILD_BUG_ON vs BUG() depending on size_is_constant parameter. It could
> use BUG() also if the compiler is LLVM < 11 or something. What would be
> the proper code for this condition?

This should work I think:

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 9d316aac0aba..1b653266f2aa 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -413,7 +413,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
 	if (size <=  16 * 1024 * 1024) return 24;
 	if (size <=  32 * 1024 * 1024) return 25;
 
-	if (size_is_constant)
+	if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION > 110000) && size_is_constant)
 		BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
 	else
 		BUG();

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

[Hyeonggon Yoo]

On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote:
> This should work I think:

compiled well with clang-10.0.1, clang-11.0.0,
and gcc-10.2.0 with x86_64 default config.

is the condition CONFIG_CLANG_VERSION > 110000,
not including 110000 it self?

> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 9d316aac0aba..1b653266f2aa 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -413,7 +413,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
>  	if (size <=  16 * 1024 * 1024) return 24;
>  	if (size <=  32 * 1024 * 1024) return 25;
>  
> -	if (size_is_constant)
> +	if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION > 110000) && size_is_constant)
>  		BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
>  	else
>  		BUG();

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

[Nathan Chancellor]

On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote:
> On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote:
>> This should work I think:
> 
> compiled well with clang-10.0.1, clang-11.0.0,
> and gcc-10.2.0 with x86_64 default config.
> 
> is the condition CONFIG_CLANG_VERSION > 110000,
> not including 110000 it self?

Ah sorry, that should definitely be >= :(

That is what I get for writing an email that late... in reality, it 
probably won't matter due to the availability of 11.0.1 and 11.1.0 but 
it should absolutely be changed.

I have not given Nick's patch a go yet but would something like this be 
acceptable? If so, did you want me to send a formal fixup patch or did 
you want to send a v4? I have no personal preference.

>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>> index 9d316aac0aba..1b653266f2aa 100644
>> --- a/include/linux/slab.h
>> +++ b/include/linux/slab.h
>> @@ -413,7 +413,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
>>   	if (size <=  16 * 1024 * 1024) return 24;
>>   	if (size <=  32 * 1024 * 1024) return 25;
>>   
>> -	if (size_is_constant)
>> +	if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION > 110000) && size_is_constant)
>>   		BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
>>   	else
>>   		BUG();

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

[Hyeonggon Yoo]

On Mon, May 17, 2021 at 05:43:22PM -0700, Nathan Chancellor wrote:
> Ah sorry, that should definitely be >= :(
>
> That is what I get for writing an email that late... in reality, it probably
> won't matter due to the availability of 11.0.1 and 11.1.0 but it should
> absolutely be changed.

> I have not given Nick's patch a go yet but would something like this be
> acceptable? If so, did you want me to send a formal fixup patch or did you
> want to send a v4? I have no personal preference.

I think fixup patch patch will be better as we can undo it later.
I don't think Nick's patch is needed because that code is not related with
clang version, and we don't need that code even in clang 10.

then is there something I can help for now?

thanks,
Hyeonggon

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

[Vlastimil Babka]

On 5/18/21 2:43 AM, Nathan Chancellor wrote:
> On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote:
>> On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote:
>>> This should work I think:
>>
>> compiled well with clang-10.0.1, clang-11.0.0,
>> and gcc-10.2.0 with x86_64 default config.
>>
>> is the condition CONFIG_CLANG_VERSION > 110000,
>> not including 110000 it self?

Good spot.

> Ah sorry, that should definitely be >= :(
> 
> That is what I get for writing an email that late... in reality, it probably
> won't matter due to the availability of 11.0.1 and 11.1.0 but it should
> absolutely be changed.
> 
> I have not given Nick's patch a go yet but would something like this be
> acceptable?

Yes.

> If so, did you want me to send a formal fixup patch or did you want
> to send a v4? I have no personal preference.

At this point a fixup is the usual way. Andrew might squash it to the original
patch (also with Marco's fixup) before sending to Linus.

>>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>>> index 9d316aac0aba..1b653266f2aa 100644
>>> --- a/include/linux/slab.h
>>> +++ b/include/linux/slab.h
>>> @@ -413,7 +413,7 @@ static __always_inline unsigned int
>>> __kmalloc_index(size_t size,
>>>       if (size <=  16 * 1024 * 1024) return 24;
>>>       if (size <=  32 * 1024 * 1024) return 25;
>>>   -    if (size_is_constant)
>>> +    if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION > 110000) &&
>>> size_is_constant)
>>>           BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
>>>       else
>>>           BUG();
> 

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

[Hyeonggon Yoo]

On Tue, May 18, 2021 at 11:28:17AM +0200, Vlastimil Babka wrote:
> On 5/18/21 2:43 AM, Nathan Chancellor wrote:
> > On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote:
> >> On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote:
> >>> This should work I think:
> >>
> >> compiled well with clang-10.0.1, clang-11.0.0,
> >> and gcc-10.2.0 with x86_64 default config.
> >>
> >> is the condition CONFIG_CLANG_VERSION > 110000,
> >> not including 110000 it self?
> 
> Good spot.

Thanks!

> > Ah sorry, that should definitely be >= :(
> > 
> > That is what I get for writing an email that late... in reality, it probably
> > won't matter due to the availability of 11.0.1 and 11.1.0 but it should
> > absolutely be changed.
> > 
> > I have not given Nick's patch a go yet but would something like this be
> > acceptable?
> 
> Yes.

You mean Nick's patch to added with Nathan's code?
I'm not sure we need this, but will add it if you can accept it.

I'll send fixup patch soon. tell me if I can improve
anything on it.

Thanks,
Hyeonggon

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

[Vlastimil Babka]

On 5/18/21 1:18 PM, Hyeonggon Yoo wrote:
> On Tue, May 18, 2021 at 11:28:17AM +0200, Vlastimil Babka wrote:
>> On 5/18/21 2:43 AM, Nathan Chancellor wrote:
>> > On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote:
>> >> On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote:
>> >>> This should work I think:
>> >>
>> >> compiled well with clang-10.0.1, clang-11.0.0,
>> >> and gcc-10.2.0 with x86_64 default config.
>> >>
>> >> is the condition CONFIG_CLANG_VERSION > 110000,
>> >> not including 110000 it self?
>> 
>> Good spot.
> 
> Thanks!
> 
>> > Ah sorry, that should definitely be >= :(
>> > 
>> > That is what I get for writing an email that late... in reality, it probably
>> > won't matter due to the availability of 11.0.1 and 11.1.0 but it should
>> > absolutely be changed.
>> > 
>> > I have not given Nick's patch a go yet but would something like this be
>> > acceptable?
>> 
>> Yes.
> 
> You mean Nick's patch to added with Nathan's code?

No, I thought Nathan was asking about his own proposal. I don't think Nick's
patch that adds 26 index solves the issue. Nathan's proposal fixed with '>=' is OK.

> I'm not sure we need this, but will add it if you can accept it.
> 
> I'll send fixup patch soon. tell me if I can improve
> anything on it.
> 
> Thanks,
> Hyeonggon
> 

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

[Hyeonggon Yoo]

On Tue, May 18, 2021 at 01:34:07PM +0200, Vlastimil Babka wrote:
> On 5/18/21 1:18 PM, Hyeonggon Yoo wrote:
> > On Tue, May 18, 2021 at 11:28:17AM +0200, Vlastimil Babka wrote:
> >> On 5/18/21 2:43 AM, Nathan Chancellor wrote:
> >> > On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote:
> >> >> On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote:
> >> >>> This should work I think:
> >> >>
> >> >> compiled well with clang-10.0.1, clang-11.0.0,
> >> >> and gcc-10.2.0 with x86_64 default config.
> >> >>
> >> >> is the condition CONFIG_CLANG_VERSION > 110000,
> >> >> not including 110000 it self?
> >> 
> >> Good spot.
> > 
> > Thanks!
> > 
> >> > Ah sorry, that should definitely be >= :(
> >> > 
> >> > That is what I get for writing an email that late... in reality, it probably
> >> > won't matter due to the availability of 11.0.1 and 11.1.0 but it should
> >> > absolutely be changed.
> >> > 
> >> > I have not given Nick's patch a go yet but would something like this be
> >> > acceptable?
> >> 
> >> Yes.
> > 
> > You mean Nick's patch to added with Nathan's code?
> 
> No, I thought Nathan was asking about his own proposal. I don't think Nick's
> patch that adds 26 index solves the issue. Nathan's proposal fixed with '>=' is OK.

Ah, Okay! I sent the patch.

Thanks,
Hyeonggon