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

[yunhui cui]

Hi Jess,

On Fri, Feb 14, 2025 at 2:05 PM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>
> On 14 Feb 2025, at 04:11, yunhui cui <cuiyunhui at bytedance.com> wrote:
> >
> > Hi,
> >
> > On Fri, Feb 14, 2025 at 2:31 AM <patchwork-bot+linux-riscv at kernel.org> wrote:
> >>
> >> Hello:
> >>
> >> This patch was applied to riscv/linux.git (fixes)
> >> by Palmer Dabbelt <palmer at 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 at 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 at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>

Thanks,
Yunhui