On Tue, Jan 03, 2023 at 02:03:37PM +0000, Mark Rutland wrote:
> On Tue, Jan 03, 2023 at 01:25:35PM +0000, Mark Rutland wrote:
> > On Tue, Dec 20, 2022 at 12:08:16PM +0100, Peter Zijlstra wrote:
> > > On Mon, Dec 19, 2022 at 12:07:25PM -0800, Boqun Feng wrote:
> > > > On Mon, Dec 19, 2022 at 04:35:30PM +0100, Peter Zijlstra wrote:
> > > > > For all architectures that currently support cmpxchg_double()
> > > > > implement the cmpxchg128() family of functions that is basically the
> > > > > same but with a saner interface.
> > > > >
> > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> > > > > ---
> > > > > arch/arm64/include/asm/atomic_ll_sc.h | 38 +++++++++++++++++++++++
> > > > > arch/arm64/include/asm/atomic_lse.h | 33 +++++++++++++++++++-
> > > > > arch/arm64/include/asm/cmpxchg.h | 26 ++++++++++++++++
> > > > > arch/s390/include/asm/cmpxchg.h | 33 ++++++++++++++++++++
> > > > > arch/x86/include/asm/cmpxchg_32.h | 3 +
> > > > > arch/x86/include/asm/cmpxchg_64.h | 55 +++++++++++++++++++++++++++++++++-
> > > > > 6 files changed, 185 insertions(+), 3 deletions(-)
> > > > >
> > > > > --- a/arch/arm64/include/asm/atomic_ll_sc.h
> > > > > +++ b/arch/arm64/include/asm/atomic_ll_sc.h
> > > > > @@ -326,6 +326,44 @@ __CMPXCHG_DBL( , , , )
> > > > > __CMPXCHG_DBL(_mb, dmb ish, l, "memory")
> > > > >
> > > > > #undef __CMPXCHG_DBL
> > > > > +
> > > > > +union __u128_halves {
> > > > > + u128 full;
> > > > > + struct {
> > > > > + u64 low, high;
> > > > > + };
> > > > > +};
> > > > > +
> > > > > +#define __CMPXCHG128(name, mb, rel, cl) \
> > > > > +static __always_inline u128 \
> > > > > +__ll_sc__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new) \
> > > > > +{ \
> > > > > + union __u128_halves r, o = { .full = (old) }, \
> > > > > + n = { .full = (new) }; \
> > > > > + \
> > > > > + asm volatile("// __cmpxchg128" #name "\n" \
> > > > > + " prfm pstl1strm, %2\n" \
> > > > > + "1: ldxp %0, %1, %2\n" \
> > > > > + " eor %3, %0, %3\n" \
> > > > > + " eor %4, %1, %4\n" \
> > > > > + " orr %3, %4, %3\n" \
> > > > > + " cbnz %3, 2f\n" \
> > > > > + " st" #rel "xp %w3, %5, %6, %2\n" \
> > > > > + " cbnz %w3, 1b\n" \
> > > > > + " " #mb "\n" \
> > > > > + "2:" \
> > > > > + : "=&r" (r.low), "=&r" (r.high), "+Q" (*(unsigned long *)ptr) \
> > > >
> > > > I wonder whether we should use "(*(u128 *)ptr)" instead of "(*(unsigned
> > > > long *) ptr)"? Because compilers may think only 64bit value pointed by
> > > > "ptr" gets modified, and they are allowed to do "useful" optimization.
> > >
> > > In this I've copied the existing cmpxchg_double() code; I'll have to let
> > > the arch folks speak here, I've no clue.
> >
> > We definitely need to ensure the compiler sees we poke the whole thing, or it
> > can get this horribly wrong, so that is a latent bug.
> >
> > See commit:
> >
> > fee960bed5e857eb ("arm64: xchg: hazard against entire exchange variable")
> >
> > ... for examples of GCC being clever, where I overlooked the *_double() cases.
> Using __uint128_t instead, e.g.
>
> diff --git a/arch/arm64/include/asm/atomic_ll_sc.h b/arch/arm64/include/asm/atomic_ll_sc.h
> index 0890e4f568fb7..cbb3d961123b1 100644
> --- a/arch/arm64/include/asm/atomic_ll_sc.h
> +++ b/arch/arm64/include/asm/atomic_ll_sc.h
> @@ -315,7 +315,7 @@ __ll_sc__cmpxchg_double##name(unsigned long old1, \
> " cbnz %w0, 1b\n" \
> " " #mb "\n" \
> "2:" \
> - : "=&r" (tmp), "=&r" (ret), "+Q" (*(unsigned long *)ptr) \
> + : "=&r" (tmp), "=&r" (ret), "+Q" (*(__uint128_t *)ptr) \
> : "r" (old1), "r" (old2), "r" (new1), "r" (new2) \
> : cl); \
> \
> diff --git a/arch/arm64/include/asm/atomic_lse.h b/arch/arm64/include/asm/atomic_lse.h
> index 52075e93de6c0..a94d6dacc0292 100644
> --- a/arch/arm64/include/asm/atomic_lse.h
> +++ b/arch/arm64/include/asm/atomic_lse.h
> @@ -311,7 +311,7 @@ __lse__cmpxchg_double##name(unsigned long old1, \
> " eor %[old2], %[old2], %[oldval2]\n" \
> " orr %[old1], %[old1], %[old2]" \
> : [old1] "+&r" (x0), [old2] "+&r" (x1), \
> - [v] "+Q" (*(unsigned long *)ptr) \
> + [v] "+Q" (*(__uint128_t *)ptr) \
> : [new1] "r" (x2), [new2] "r" (x3), [ptr] "r" (x4), \
> [oldval1] "r" (oldval1), [oldval2] "r" (oldval2) \
> : cl); \
>
> ... makes GCC much happier:
> ... I'll go check whether clang is happy with that, and how far back that can
> go, otherwise we'll need to blat the high half with a separate constaint that
> (ideally) doesn't end up allocating a pointless address register.
Hmm... from the commit history it looks like GCC prior to 5.1 might not be
happy with that, but that *might* just be if we actually do arithmetic on the
value, and we might be ok just using it for memroy effects. I can't currently
get such an old GCC to run on my machines so I haven't been able to check.
I'll dig into this a bit more tomorrow, but it looks like the options (for a
backport-suitable fix) will be:
(a) use a __uint128_t input+output, as above, if we're lucky
(b) introduce a second 64-bit input+output for the high half (likely a "+o")
(c) use a full memory clobber for ancient compilers.
Mark.
