Messages in this thread | | | Date | Fri, 26 Nov 2021 17:25:04 +0100 | Subject | Re: [PATCH] powerpc: mm: radix_tlb: rearrange the if-else block | From | Christophe Leroy <> |
| |
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"); >
| |