On Sat, May 7, 2022 at 9:04 AM Christophe Leroy
<christophe.le...@csgroup.eu> wrote:
> Le 02/05/2022 à 15:24, Michael Ellerman a écrit :
> > CGEL <cgel....@gmail.com> writes:
> >> From: Jing Yangyang <jing.yangy...@zte.com.cn>
> >>
> >> Use BUG_ON instead of a if condition followed by BUG.
> >>
> >> ./arch/powerpc/include/asm/book3s/64/pkeys.h:21:2-5:WARNING
> >> Use BUG_ON instead of if condition followed by BUG.
> >> ./arch/powerpc/include/asm/book3s/64/pkeys.h:14:2-5:WARNING
> >> Use BUG_ON instead of if condition followed by BUG.
> >>
> >> Generated by: scripts/coccinelle/misc/bugon.cocci
> >>
> >> Reported-by: Zeal Robot <zea...@zte.com.cn>
> >> Signed-off-by: Jing Yangyang <jing.yangy...@zte.com.cn>
> >> ---
> >> arch/powerpc/include/asm/book3s/64/pkeys.h | 6 ++----
> >> 1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/book3s/64/pkeys.h
> >> b/arch/powerpc/include/asm/book3s/64/pkeys.h
> >> index 5b17813..5f74f0c 100644
> >> --- a/arch/powerpc/include/asm/book3s/64/pkeys.h
> >> +++ b/arch/powerpc/include/asm/book3s/64/pkeys.h
> >> @@ -10,15 +10,13 @@ static inline u64 vmflag_to_pte_pkey_bits(u64 vm_flags)
> >> if (!mmu_has_feature(MMU_FTR_PKEY))
> >> return 0x0UL;
> >>
> >> - if (radix_enabled())
> >> - BUG();
> >> + BUG_ON(radix_enabled());
> >> return hash__vmflag_to_pte_pkey_bits(vm_flags);
> >> }
> >>
> >> static inline u16 pte_to_pkey_bits(u64 pteflags)
> >> {
> >> - if (radix_enabled())
> >> - BUG();
> >> + BUG_ON(radix_enabled());
> >> return hash__pte_to_pkey_bits(pteflags);
> >> }
> >
> > Have you checked how this changes the generated code?
> >
> > radix_enabled() is a jump label, via mmu_feature().
> >
> > Possibly the compiler just works it all out and generates the same code,
> > but I'd want some evidence of that before merging this.
>
> Seems like the compiler is not that good, the generated code for test1()
> is much better than the one for test2(), see below.
>
> void test1(void)
> {
> if (radix_enabled())
> BUG();
> }
>
> void test2(void)
> {
> BUG_ON(radix_enabled());
> }
>
> 0000000000000900 <.test1>:
> 900: 60 00 00 00 nop
> 904: 0f e0 00 00 twui r0,0
> 908: 60 00 00 00 nop
> 90c: 60 00 00 00 nop
> 910: 4e 80 00 20 blr
> 914: 60 00 00 00 nop
> 918: 60 00 00 00 nop
> 91c: 60 00 00 00 nop
>
> 0000000000000920 <.test2>:
> 920: 60 00 00 00 nop
> 924: 39 20 00 01 li r9,1
> 928: 0b 09 00 00 tdnei r9,0
> 92c: 4e 80 00 20 blr
> 930: 39 20 00 00 li r9,0
> 934: 0b 09 00 00 tdnei r9,0
> 938: 4e 80 00 20 blr
>
>
> We should keep things as they are and change the coccinelle script.
Maybe just drop the custom ppc64 BUG_ON() then if it creates worse
code? The default BUG_ON() should be equivalent to the open-coded
version.
Arnd
