Re: [PATCH] arm64: atomics: Fix the issue on xchg when switch to atomic instruction

From: Shaokun Zhang <zhangshaokun@hisilicon.com>
To: Will Deacon <will@kernel.org>
Cc: Yuqi Jin <jinyuqi@huawei.com>,
	Andrew Murray <amurray@thegoodpenguin.co.uk>,
	linux-arm-kernel@lists.infradead.org,
	Catalin Marinas <catalin.marinas@arm.com>
Subject: Re: [PATCH] arm64: atomics: Fix the issue on xchg when switch to atomic instruction
Date: Wed, 6 May 2020 18:39:16 +0800	[thread overview]
Message-ID: <90dde2e8-ea11-fa7b-1a44-4d357a61cd66@hisilicon.com> (raw)
In-Reply-To: <20200506075352.GE7021@willie-the-truck>

Hi Will,

Apologies for my noise, you are right and it's my mistake.

On 2020/5/6 15:53, Will Deacon wrote:
> On Wed, May 06, 2020 at 03:00:39PM +0800, Shaokun Zhang wrote:
>> On 2020/5/5 17:15, Will Deacon wrote:
>>> On Tue, May 05, 2020 at 05:02:35PM +0800, Shaokun Zhang wrote:
>>>> From: Yuqi Jin <jinyuqi@huawei.com>
>>>>
>>>> Since commit addfc38672c7 ("arm64: atomics: avoid out-of-line ll/sc atomics"),
>>>> it has provided inline implementations of both LSE and ll/sc and used a static
>>>> key to select between them, which allows the compiler to generate better
>>>> atomics code.
>>>> However, xchg still uses the original method which would fail to switch to
>>>> the atomic instruction correctly, Let's fix this issue.
>>>
>>> Please can you elaborate on the failure mode? The current code looks alright
>>
>> When enable CONFIG_ARM64_LSE_ATOMICS, xchg is failed to switch to swp instruction
>> or dynamic replacement instructions are not seen.
>>
>> We do some tests on the copy of xchg_tail,:
>> u32 xchg_tail_my(struct qspinlock *lock, u32 tail)
>> {
>>         return (u32)xchg_relaxed(&lock->tail,
>>                                  tail >> _Q_TAIL_OFFSET) << _Q_TAIL_OFFSET;
>> }
>> and the asm code is as follows:
>>
>> ffff80001015b050 <xchg_tail_my>:
>> ffff80001015b050:       a9be7bfd        stp     x29, x30, [sp, #-32]!
>> ffff80001015b054:       910003fd        mov     x29, sp
>> ffff80001015b058:       a90153f3        stp     x19, x20, [sp, #16]
>> ffff80001015b05c:       2a0103f3        mov     w19, w1
>> ffff80001015b060:       aa0003f4        mov     x20, x0
>> ffff80001015b064:       aa1e03e0        mov     x0, x30
>> ffff80001015b068:       97fd07ee        bl      ffff80001009d020 <_mcount>
>> ffff80001015b06c:       53107e61        lsr     w1, w19, #16
>> ffff80001015b070:       91000a83        add     x3, x20, #0x2
>> ffff80001015b074:       f9800071        prfm    pstl1strm, [x3]
>> ffff80001015b078:       485f7c60        ldxrh   w0, [x3]
>> ffff80001015b07c:       48027c61        stxrh   w2, w1, [x3]
>> ffff80001015b080:       35ffffc2        cbnz    w2, ffff80001015b078 <xchg_tail_my+0x28>
>> ffff80001015b084:       53103c00        lsl     w0, w0, #16
>> ffff80001015b088:       a94153f3        ldp     x19, x20, [sp, #16]
>> ffff80001015b08c:       a8c27bfd        ldp     x29, x30, [sp], #32
>> ffff80001015b090:       d65f03c0        ret
> 
> This should get patched at runtime, but you're saying that's not happening?
> 

My mistake, I didn't check the runtime carefully.

>>> to me, so I'm clearly missing something. What's broken?
>>>
>>
>> I'm not sure whether the ARM64_LSE_ATOMIC_INSN could works correctly after the
>> commit addfc38672c7. If we implement xchg using __lse_ll_sc_body like cmpxchg_case,
>> xchg works ok.
>>
>> What's more, I am wondering why xchg still uses the dynamic replacement mode,
>> but cmpxchg uses another mode. ;-)
> 
> There's a trade-off involving the number of clobbered registers and the
> number of instructions, which made a bit more sense when we used to branch
> out-of-line. We also do the direct patching for the pcpu atomics.
> 

Thanks your explanation, got it and I did check pcpu atomics before.

Thanks,
Shaokun

> Will
> 
> .
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel