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

Jessica Clarke
2025-02-14T07:17:30Z

On 14 Feb 2025, at 06:22, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> 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?

Because there are multiple ways in which compilers can emit code that
does not keep values sign-extended in registers within a function. The
calling convention only provides that guarantee on entry to a function
following the standard calling convention. In the specific instance of
compiling the version of Linux you are compiling with the kernel config
you are using and the precise compiler you are using, the patch happens
to not be needed for this snippet of the binary because the generated
code already has the sign-extended value in the register used. But just
because there are instances when the patch does nothing doesn’t mean
all instances are like that.

Jess

>> 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