LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Nathan Chancellor <nathan@kernel.org>, Arnd Bergmann <arnd@arndb.de>
Cc: Anders Roxell <anders.roxell@linaro.org>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	llvm@lists.linux.dev, Nick Desaulniers <ndesaulniers@google.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH] powerpc: mm: radix_tlb: rearrange the if-else block
Date: Fri, 26 Nov 2021 17:25:04 +0100	[thread overview]
Message-ID: <90ea33c6-2e93-ea19-3052-90e15979578f@csgroup.eu> (raw)
In-Reply-To: <YaEBTbjGyUBmISGK@archlinux-ax161>



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");
> 

      reply	other threads:[~2021-11-26 16:27 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-25 15:44 Anders Roxell
2021-11-26 13:43 ` Christophe Leroy
2021-11-26 13:59   ` Arnd Bergmann
2021-11-26 15:46     ` Nathan Chancellor
2021-11-26 16:25       ` Christophe Leroy [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=90ea33c6-2e93-ea19-3052-90e15979578f@csgroup.eu \
    --to=christophe.leroy@csgroup.eu \
    --cc=anders.roxell@linaro.org \
    --cc=arnd@arndb.de \
    --cc=benh@kernel.crashing.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=llvm@lists.linux.dev \
    --cc=nathan@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=paulus@samba.org \
    --subject='Re: [PATCH] powerpc: mm: radix_tlb: rearrange the if-else block' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).