Re: [PATCH] arm64: futex: Consolidate 'old == new' check in __lsui_cmpxchg32()

[Yeoreum Yun <yeoreum.yun@arm.com>]

> On Tue, May 19, 2026 at 04:09:02PM +0100, Catalin Marinas wrote:
> > On Tue, May 19, 2026 at 10:08:22AM +0100, Will Deacon wrote:
> > > The LSUI futex implementation relies on a cmpxchg() loop to implement
> > > FUTEX_OP_XOR, as the architecture doesn't provide unprivileged *EOR
> > > atomics. Since the unprivileged 'CAST' instructions used to implement
> > > the cmpxchg() can only operate on 64-bit memory locations, the
> > > __lsui_cmpxchg32() helper function performs a song and dance to marshall
> > > the 32-bit futex value into the correct part of a 64-bit register and
> > > fill the remaining bytes with the neighbouring data.
> > 
> > IIRC, the reason for the current __lsui_cmpxchg32() was not EOR but the
> > expected futex_atomic_cmpxchg_inatomic() semantics. Looking at it again,
> > we have wake_futex_pi() that does something else if the ret is 0 but the
> > value differs. Looking at it again, the caller of wake_futex_pi()
> > retries on -EAGAIN anyway, so I don't see a correctness issue, it will
> > eventually hit the condition.
> 
> Hmm, but I think that means my patch does change the behaviour of
> wake_futex_pi() in an undesirable way. For example, futex_unlock_pi()
> will go round the retry loop for any change in the futex value, whereas
> before we would go back to userspace only if the TID changed.
> 
> So I think we should swallow the -EAGAIN for the CAS-based cmpxchg() if
> the futex word has changed, along the lines of the diff below.
> 
> Will
> 
> --->8
> 
> diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
> index db84a7b2de74..79c6d86c38a9 100644
> --- a/arch/arm64/include/asm/futex.h
> +++ b/arch/arm64/include/asm/futex.h
> @@ -215,14 +215,14 @@ __lsui_futex_atomic_eor(int oparg, u32 __user *uaddr, int *oval)
>  static __always_inline int
>  __lsui_futex_cmpxchg(u32 __user *uaddr, u32 oldval, u32 newval, u32 *oval)
>  {
> +       u32 curval = oldval;
>         int ret;
>  
> -       /*
> -        * Callers of futex_atomic_cmpxchg_inatomic() already retry on
> -        * -EAGAIN, no need for another loop of max retries.
> -        */
> -       ret = __lsui_cmpxchg32(uaddr, &oldval, newval);
> -       *oval = oldval;
> +       ret = __lsui_cmpxchg32(uaddr, &curval, newval);
> +       if (ret == -EAGAIN && curval != oldval)
> +               ret = 0;
> +
> +       *oval = curval;
>         return ret;
>  }
>  #endif /* CONFIG_ARM64_LSUI */

Agree. It is good that this additional patch does not change
the existing behavior.

@Catalin, Could you check this please?

Thanks!

-- 
Sincerely,
Yeoreum Yun