powerpc/tlb: Implement book3s/32/tlbflush.h local_flush_tlb_page_psize

The commit introducing this function implemented it as a build bug on this
platform to make the compiler happy, as the only use in the code is guarded
behind a radix_enabled() conditional.

GCC recognises that cpu_has_feature(MMU_FTR_TYPE_RADIX) returns false on this
platform and eliminates the build bug as dead code. However, when
CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG is enabled, the assertion and possible
call to early_cpu_... prevents Clang from eliminating any code that's
conditional on the return value. So Clang triggers the build bug as reported
by the kernel test robot:

https://lore.kernel.org/llvm/202301212348.eDkowvfF-lkp@intel.com

Fixes: 274d842fa1ef ("powerpc/tlb: Add local flush for page given mm_struct and psize")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---

Supersedes https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20230124215424.9068-1-bgray@linux.ibm.com/

---
 arch/powerpc/include/asm/book3s/32/tlbflush.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)


base-commit: ca272751ba18ca8f137af631cbc9f3f987fab6e3
--
2.39.1

Le 31/01/2023 à 03:58, Benjamin Gray a écrit :
> The commit introducing this function implemented it as a build bug on this
> platform to make the compiler happy, as the only use in the code is guarded
> behind a radix_enabled() conditional.
> 
> GCC recognises that cpu_has_feature(MMU_FTR_TYPE_RADIX) returns false on this
> platform and eliminates the build bug as dead code. However, when
> CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG is enabled, the assertion and possible
> call to early_cpu_... prevents Clang from eliminating any code that's
> conditional on the return value. So Clang triggers the build bug as reported
> by the kernel test robot:

I still think it is not the correct fix. You are putting the problem 
under the carpet instead of fixing it. There are many other places where 
radix_enabled() or other mmu_has_feature() are used with the expectation 
that one leg will be eliminated at build time.

As written in previous thread, have you considered reworking 
mmu_has_feature() to move the CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG 
after the below block:

	if (MMU_FTRS_ALWAYS & feature)
		return true;

	if (!(MMU_FTRS_POSSIBLE & feature))
		return false;


And as this looks like a Clang bug or limitation, can you provide us 
with a link to the Clang issue you have opened for it ?


Looking into it in more details, I'm even more puzzled. As far as I can 
see, local_flush_tlb_page_psize() is used only at one place, that is 
function __do_patch_instruction_mm(). So if Clang fails to identify it 
as a dead leg, it is the full __do_patch_instruction_mm() which is kept 
for no reason.

On the other hand, I see that local_flush_tlb_page_psize() implemented 
for nohash/32, so yes we can also implement it for book3s/32. But then 
the commit log should explain things differently.

By the way, I also see that local_flush_tlb_page_psize() for book3s/64 
does just nothing at all for non Radix. Is that correct ?

Thanks
Christophe


> 
> https://lore.kernel.org/llvm/202301212348.eDkowvfF-lkp@intel.com
> 
> Fixes: 274d842fa1ef ("powerpc/tlb: Add local flush for page given mm_struct and psize")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
> 
> Supersedes https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20230124215424.9068-1-bgray@linux.ibm.com/
> 
> ---
>   arch/powerpc/include/asm/book3s/32/tlbflush.h | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/32/tlbflush.h b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> index 4be572908124..cde3b6f5d563 100644
> --- a/arch/powerpc/include/asm/book3s/32/tlbflush.h
> +++ b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> @@ -2,8 +2,6 @@
>   #ifndef _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
>   #define _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
> 
> -#include <linux/build_bug.h>
> -
>   #define MMU_NO_CONTEXT      (0)
>   /*
>    * TLB flushing for "classic" hash-MMU 32-bit CPUs, 6xx, 7xx, 7xxx
> @@ -80,7 +78,7 @@ static inline void local_flush_tlb_page(struct vm_area_struct *vma,
>   static inline void local_flush_tlb_page_psize(struct mm_struct *mm,
>   					      unsigned long vmaddr, int psize)
>   {
> -	BUILD_BUG();
> +	flush_range(mm, vmaddr, vmaddr + PAGE_SIZE);
>   }
> 
>   static inline void local_flush_tlb_mm(struct mm_struct *mm)
> 
> base-commit: ca272751ba18ca8f137af631cbc9f3f987fab6e3
> --
> 2.39.1

On Tue, 2023-01-31 at 06:33 +0000, Christophe Leroy wrote:
> I still think it is not the correct fix. You are putting the problem 
> under the carpet instead of fixing it. There are many other places
> where 
> radix_enabled() or other mmu_has_feature() are used with the
> expectation 
> that one leg will be eliminated at build time.

And none of them are actively causing build failures AFAIK. I agree
that there may be a pre-existing optimisation problem, but I'm not
trying to address it in this patch. I'm just trying to fix the build I
broke. As such I haven't opened an issue with Clang yet either.

> As written in previous thread, have you considered reworking 
> mmu_has_feature() to move the CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG 
> after the below block:
> 
>         if (MMU_FTRS_ALWAYS & feature)
>                 return true;
> 
>         if (!(MMU_FTRS_POSSIBLE & feature))
>                 return false;
> 

Yes, I did. I also discussed with Michael Ellerman what he would
prefer, and he indicated he still would still like to just implement
the function.


> Looking into it in more details, I'm even more puzzled. As far as I
> can 
> see, local_flush_tlb_page_psize() is used only at one place, that is 
> function __do_patch_instruction_mm(). So if Clang fails to identify
> it 
> as a dead leg, it is the full __do_patch_instruction_mm() which is
> kept 
> for no reason.

Right, because that is the function that's guarded behind
radix_enabled().

> By the way, I also see that local_flush_tlb_page_psize() for
> book3s/64 
> does just nothing at all for non Radix. Is that correct ?

That is how the other local page flushes are implemented. If there's
some undocumented exception here I'd be relying on review on the list
from people who have experience with details of how Hash mmu is handled
on this platform.

Le 31/01/2023 à 22:58, Benjamin Gray a écrit :
> On Tue, 2023-01-31 at 06:33 +0000, Christophe Leroy wrote:
>> I still think it is not the correct fix. You are putting the problem
>> under the carpet instead of fixing it. There are many other places
>> where
>> radix_enabled() or other mmu_has_feature() are used with the
>> expectation
>> that one leg will be eliminated at build time.
> 
> And none of them are actively causing build failures AFAIK. I agree
> that there may be a pre-existing optimisation problem, but I'm not
> trying to address it in this patch. I'm just trying to fix the build I
> broke. As such I haven't opened an issue with Clang yet either.
> 
>> As written in previous thread, have you considered reworking
>> mmu_has_feature() to move the CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG
>> after the below block:
>>
>>          if (MMU_FTRS_ALWAYS & feature)
>>                  return true;
>>
>>          if (!(MMU_FTRS_POSSIBLE & feature))
>>                  return false;
>>
> 
> Yes, I did. I also discussed with Michael Ellerman what he would
> prefer, and he indicated he still would still like to just implement
> the function.

I'm fine with that. But if it is the intention, don't focus on the bug 
it fixes, but more on what it implements and how. That's what I would 
like to read in the commit message. It is nice to explain that there is 
a problem with CLANG and what the problem is, but only as side benefit 
of the commit.

For instance, explain why you can use flush_range() to implement it, and 
why you use PAGE_SIZE and not psize, etc ...

Christophe