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

From: Will Deacon <will@kernel.org>
To: Shaokun Zhang <zhangshaokun@hisilicon.com>
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 11:44:36 +0100	[thread overview]
Message-ID: <20200506104435.GB8043@willie-the-truck> (raw)
In-Reply-To: <90dde2e8-ea11-fa7b-1a44-4d357a61cd66@hisilicon.com>

On Wed, May 06, 2020 at 06:39:16PM +0800, Shaokun Zhang wrote:
> Apologies for my noise, you are right and it's my mistake.

No need to apologise, but thanks for letting me know.

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

Good to hear there's not a bug, but if you see a performance benefit from
using the static-key for xchg() then I'd obviously be open to changing it
over as well.

Thanks,

Will

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