Re: [PATCH] powerpc/tlb: Remove BUILD_BUG for book3s/32/tlbflush.h local_flush_tlb_page_psize

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 24/01/2023 à 22:54, Benjamin Gray a écrit :
> Converts the BUILD_BUG to a WARN to allow building with a low/unoptimised
> compiler.

No no no no. Please don't do that.

That approach is used everywhere in the kernel, why should it be a 
problem only for local_flush_tlb_page_psize() and not everywhere else ?

Really if it doesn't work it means there is a deep problem in your 
compiler. We really don't want the overhead of WARN over BUILD_BUG in 
the normal case.

By the way, are you should the problem is really BUILD_BUG() ? Looking 
at your patch I would think that the problem is because it is "static 
inline". Have you tried 'static __always_inline' instead ?

Christophe

> 
> The original expectation was that a compiler would see that the only
> usage of this function was in a function that is only called behind
> radix-only guards. And it worked this way on GCC. It seems Clang does
> not optimise away this call however, so thinks the function may be
> invoked and triggers the build bug as reported by the kernel test robot.
> 
> https://lore.kernel.org/llvm/202301212348.eDkowvfF-lkp@intel.com
> 
> This fix converts the build bug to a warning to allow builds without
> relying on particular compiler optimisation behaviours. The warning is
> not rate limited because this implementation should still never be called
> as-is, and anyone who might invoke it might appreciate it being very
> obvious that it's not behaving as expected.
> 
> 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>
> ---
>   arch/powerpc/include/asm/book3s/32/tlbflush.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/32/tlbflush.h b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> index 4be572908124..675196884640 100644
> --- a/arch/powerpc/include/asm/book3s/32/tlbflush.h
> +++ b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> @@ -2,7 +2,7 @@
>   #ifndef _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
>   #define _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
>   
> -#include <linux/build_bug.h>
> +#include <linux/bug.h>
>   
>   #define MMU_NO_CONTEXT      (0)
>   /*
> @@ -80,7 +80,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();
> +	WARN(1, "Unimplemented local TLB flush with psize");
>   }
>   
>   static inline void local_flush_tlb_mm(struct mm_struct *mm)
> 
> base-commit: 53ab112a95086d10dc353ea4f979abb01644bbb6