lkml.org 
[lkml]   [2021]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] powerpc: mm: radix_tlb: rearrange the if-else block
From


Le 26/11/2021 à 16:46, Nathan Chancellor a écrit :
> On Fri, Nov 26, 2021 at 02:59:29PM +0100, Arnd Bergmann wrote:
>> On Fri, Nov 26, 2021 at 2:43 PM Christophe Leroy
>> <christophe.leroy@csgroup.eu> wrote:
>>> Le 25/11/2021 à 16:44, Anders Roxell a écrit :
>>> Can't you fix CLANG instead :) ?
>>>
>>> Or just add an else to the IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) that
>>> sets hstart and hend to 0 ?
>>
>> That doesn't sound any less risky than duplicating the code, it can lead to
>> incorrect changes just as easily if a patch ends up actually flushing at the
>> wrong address, and the compiler fails to complain because of the bogus
>> initialization.
>>
>>> Or just put hstart and hend calculation outside the IS_ENABLED() ? After
>>> all GCC should drop the calculation when not used.
>>
>> I like this one. I'm still unsure how clang can get so confused about whether
>> the variables are initialized or not, usually it handles this much better than
>> gcc. My best guess is that one of the memory clobbers makes it conclude
>> that 'hflush' can be true when it gets written to by an inline asm.
>
> As far as I am aware, clang's analysis does not evaluate variables when
> generating a control flow graph and using that for static analysis:
>
> https://godbolt.org/z/PdGxoq9j7
>
> Based on the control flow graph, it knows that hstart and hend are
> uninitialized because IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) gets
> expanded to 0 by the preprocessor but it does not seem like it can piece
> together that hflush's value of false is only changed to true under the
> now 'if (0) {' branch, meaning that all the calls to __tlbiel_va_range()
> never get evaluated. That may or may not be easy to fix in clang but we
> run into issues like this so infrequently.
>
> At any rate, the below diff works for me.
>
> Cheers,
> Nathan
>
> diff --git a/arch/powerpc/mm/book3s64/radix_tlb.c b/arch/powerpc/mm/book3s64/radix_tlb.c
> index 7724af19ed7e..156a631df976 100644
> --- a/arch/powerpc/mm/book3s64/radix_tlb.c
> +++ b/arch/powerpc/mm/book3s64/radix_tlb.c
> @@ -1174,12 +1174,10 @@ static inline void __radix__flush_tlb_range(struct mm_struct *mm,
> bool hflush = false;
> unsigned long hstart, hend;
>
> - if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
> - hstart = (start + PMD_SIZE - 1) & PMD_MASK;
> - hend = end & PMD_MASK;
> - if (hstart < hend)
> - hflush = true;
> - }
> + hstart = (start + PMD_SIZE - 1) & PMD_MASK;
> + hend = end & PMD_MASK;
> + if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hstart < hend)
> + hflush = true;

Yes I like that much better.

Maybe even better with

hflush = IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hstart < hend;

(And remove default false value at declaration).

>
> if (type == FLUSH_TYPE_LOCAL) {
> asm volatile("ptesync": : :"memory");
>

\
 
 \ /
  Last update: 2021-11-26 17:27    [W:0.057 / U:0.528 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site