Re: [PATCH v7 RESEND 5/6] arm64: futex: small optimisation for __llsc_futex_atomic_set()

Yeoreum Yun
2025-09-11T16:19:59Z

Hi Will,

[...]

> > arch/arm64/include/asm/futex.h | 43 ++++++++++++++++++++++++++++------
> > 1 file changed, 36 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
> > index ab7003cb4724..22a6301a9f3d 100644
> > --- a/arch/arm64/include/asm/futex.h
> > +++ b/arch/arm64/include/asm/futex.h
> > @@ -13,7 +13,7 @@
> >
> > #define LLSC_MAX_LOOPS 128 /* What's the largest number you can think of? */
> >
> > -#define LLSC_FUTEX_ATOMIC_OP(op, insn) \
> > +#define LLSC_FUTEX_ATOMIC_OP(op, asm_op) \
> > static __always_inline int \
> > __llsc_futex_atomic_##op(int oparg, u32 __user *uaddr, int *oval) \
> > { \
> > @@ -24,7 +24,7 @@ __llsc_futex_atomic_##op(int oparg, u32 __user *uaddr, int *oval) \
> > asm volatile("// __llsc_futex_atomic_" #op "\n" \
> > " prfm pstl1strm, %2\n" \
> > "1: ldxr %w1, %2\n" \
> > - insn "\n" \
> > +" " #asm_op " %w3, %w1, %w5\n" \
> > "2: stlxr %w0, %w3, %2\n" \
> > " cbz %w0, 3f\n" \
> > " sub %w4, %w4, %w0\n" \
> > @@ -46,11 +46,40 @@ __llsc_futex_atomic_##op(int oparg, u32 __user *uaddr, int *oval) \
> > return ret; \
> > }
> >
> > -LLSC_FUTEX_ATOMIC_OP(add, "add %w3, %w1, %w5")
> > -LLSC_FUTEX_ATOMIC_OP(or, "orr %w3, %w1, %w5")
> > -LLSC_FUTEX_ATOMIC_OP(and, "and %w3, %w1, %w5")
> > -LLSC_FUTEX_ATOMIC_OP(eor, "eor %w3, %w1, %w5")
> > -LLSC_FUTEX_ATOMIC_OP(set, "mov %w3, %w5")
> > +LLSC_FUTEX_ATOMIC_OP(add, add)
> > +LLSC_FUTEX_ATOMIC_OP(or, orr)
> > +LLSC_FUTEX_ATOMIC_OP(and, and)
> > +LLSC_FUTEX_ATOMIC_OP(eor, eor)
> > +
> > +static __always_inline int
> > +__llsc_futex_atomic_set(int oparg, u32 __user *uaddr, int *oval)
> > +{
> > + unsigned int loops = LLSC_MAX_LOOPS;
> > + int ret, oldval;
> > +
> > + uaccess_enable_privileged();
> > + asm volatile("//__llsc_futex_xchg\n"
> > +" prfm pstl1strm, %2\n"
> > +"1: ldxr %w1, %2\n"
> > +"2: stlxr %w0, %w4, %2\n"
> > +" cbz %w3, 3f\n"
> > +" sub %w3, %w3, %w0\n"
> > +" cbnz %w3, 1b\n"
> > +" mov %w0, %w5\n"
> > +"3:\n"
> > +" dmb ish\n"
> > + _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %w0)
> > + _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %w0)
> > + : "=&r" (ret), "=&r" (oldval), "+Q" (*uaddr), "+r" (loops)
> > + : "r" (oparg), "Ir" (-EAGAIN)
> > + : "memory");
> > + uaccess_disable_privileged();
> > +
> > + if (!ret)
> > + *oval = oldval;
>
> Hmm, I'm really not sure this is worthwhile. I doubt the "optimisation"
> actually does anything and adding a whole new block of asm just for the
> SET case isn't much of an improvement on the maintainability side, either.

TBH, I had the same question, but I thought this code seems to modify
freqenetly, I decide even a small optimisation -- reduce one instruction
only.

But I don't have strong opinion for this patch.
If it's not good for maintainability perspective,
This patch can be dropped.

Thanks!

--
Sincerely,
Yeoreum Yun