Re: [External] [PATCH] riscv/futex: sign extend compare value in atomic cmpxchg

yunhui cui
2025-02-14T06:23:04Z

Hi Jess,

On Fri, Feb 14, 2025 at 2:05 PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 14 Feb 2025, at 04:11, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >
> > Hi,
> >
> > On Fri, Feb 14, 2025 at 2:31 AM <patchwork-bot+linux-riscv@kernel.org> wrote:
> >>
> >> Hello:
> >>
> >> This patch was applied to riscv/linux.git (fixes)
> >> by Palmer Dabbelt <palmer@rivosinc.com>:
> >>
> >> On Mon, 03 Feb 2025 11:06:00 +0100 you wrote:
> >>> Make sure the compare value in the lr/sc loop is sign extended to match
> >>> what lr.w does. Fortunately, due to the compiler keeping the register
> >>> contents sign extended anyway the lack of the explicit extension didn't
> >>> result in wrong code so far, but this cannot be relied upon.
> >>>
> >>> Fixes: b90edb33010b ("RISC-V: Add futex support.")
> >>> Signed-off-by: Andreas Schwab <schwab@suse.de>
> >>>
> >>> [...]
> >>
> >> Here is the summary with links:
> >> - riscv/futex: sign extend compare value in atomic cmpxchg
> >> https://git.kernel.org/riscv/c/5c238584bce5
> >>
> >> You are awesome, thank you!
> >> --
> >> Deet-doot-dot, I am a bot.
> >> https://korg.docs.kernel.org/patchwork/pwbot.html
> >>
> >>
> >
> > I applied this patch, but it doesn't seem to take effect. There is no
> > sign extension for a2 in the assembly code. What did I miss?
> > GCC version >= 13.
> >
> > ffffffff800e87e0 <futex_atomic_cmpxchg_inatomic>:
> > ffffffff800e87e0: 1141 addi sp,sp,-16
> > ffffffff800e87e2: e422 sd s0,8(sp)
> > ffffffff800e87e4: 2bf01793 bseti a5,zero,0x3f
> > ffffffff800e87e8: 0800 addi s0,sp,16
> > ffffffff800e87ea: 17ed addi a5,a5,-5
> > ffffffff800e87ec: 00b7f663 bgeu a5,a1,ffffffff800e87f8
> > <futex_atomic_cmpxchg_inatomic+0x18>
> > ffffffff800e87f0: 6422 ld s0,8(sp)
> > ffffffff800e87f2: 5549 li a0,-14
> > ffffffff800e87f4: 0141 addi sp,sp,16
> > ffffffff800e87f6: 8082 ret
> > ffffffff800e87f8: 872a mv a4,a0
> > ffffffff800e87fa: 00040837 lui a6,0x40
> > ffffffff800e87fe: 10082073 csrs sstatus,a6
> > ffffffff800e8802: 4781 li a5,0
> > ffffffff800e8804: 1605a8af lr.w.aqrl a7,(a1)
> > ffffffff800e8808: 00c89563 bne a7,a2,ffffffff800e8812
> > <futex_atomic_cmpxchg_inatomic+0x32>
> > ffffffff800e880c: 1ed5a52f sc.w.aqrl a0,a3,(a1)
> > ffffffff800e8810: f975 bnez a0,ffffffff800e8804
> > <futex_atomic_cmpxchg_inatomic+0x24>
> > ffffffff800e8812: 0007851b sext.w a0,a5
> > ffffffff800e8816: 10083073 csrc sstatus,a6
> > ffffffff800e881a: 01172023 sw a7,0(a4)
> > ffffffff800e881e: 6422 ld s0,8(sp)
> > ffffffff800e8820: 0141 addi sp,sp,16
> > ffffffff800e8822: 8082 ret
>
> The calling convention means a2 will be sign-extended on entry to the
> function, and since your compiler has not done anything to change the
> value that will still be true. So it has (legitimately) optimised out
> any sign extension as a no-op.

If so, why this patch?

> Are you seeing any problems that you
> believe to be caused by this function, or are you just inspecting the
> disassembly to satisfy your own curiosity?

No actual problem traced here.

>
> > Should we do it like this:
> > __asm__ __volatile__ (
> > " sext.w %[ov], %[ov] \n"
> > ...
>
> No, it’s unnecessary and prevents optimisation.
>
> Jess
>
> > Thanks,
> > Yunhui
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>

Thanks,
Yunhui