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

[Catalin Marinas <catalin.marinas@arm.com>]

On Tue, Jun 16, 2026 at 10:26:59AM +0100, Yeoreum Yun wrote:
> > 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?

Ah, yes, looks good to me. I completely forgot about this.

-- 
Catalin