Re: [PATCH v15 5/8] arm64: futex: support futex with FEAT_LSUI

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

Hi Catalin,

> On Fri, Mar 13, 2026 at 09:23:58AM +0000, Yeoreum Yun wrote:
> > > On Fri, Feb 27, 2026 at 03:17:02PM +0000, Yeoreum Yun wrote:
> > > > +
> > > > +		if (__lsui_cmpxchg64(uaddr64, &oval64.raw, nval64.raw))
> > > > +			return -EFAULT;
> > > > +
> > > > +		oldval = oval64.futex[futex_pos];
> > > > +		other = oval64.futex[other_pos];
> > > > +		orig_other = orig64.futex[other_pos];
> > > > +
> > > > +		if (other == orig_other) {
> > > > +			ret = 0;
> > > > +			break;
> > > > +		}
> > >
> > > Is this check correct? What if the cmpxchg64 failed because futex_pos
> > > was changed but other_pos remained the same, it will just report success
> > > here. You need to compare the full 64-bit value to ensure the cmpxchg64
> > > succeeded.
> >
> > This is not matter since "futex_cmpxchg_value_locked()" checks
> > the "curval" and "oldval" IOW, though it returns success,
> > caller of this function always checks the "curval" and "oldval"
> > and when it's different, It handles to change return as -EAGAIN.
>
> Ah, ok, it makes sense (I did not check the callers).
>
> > > > +	}
> > > > +
> > > > +	if (!ret)
> > > > +		*oval = oldval;
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static __always_inline int
> > > > +__lsui_futex_atomic_and(int oparg, u32 __user *uaddr, int *oval)
> > > > +{
> > > > +	/*
> > > > +	 * Undo the bitwise negation applied to the oparg passed from
> > > > +	 * arch_futex_atomic_op_inuser() with FUTEX_OP_ANDN.
> > > > +	 */
> > > > +	return __lsui_futex_atomic_andnot(~oparg, uaddr, oval);
> > > > +}
> > > > +
> > > > +static __always_inline int
> > > > +__lsui_futex_atomic_eor(int oparg, u32 __user *uaddr, int *oval)
> > > > +{
> > > > +	u32 oldval, newval, val;
> > > > +	int ret, i;
> > > > +
> > > > +	if (get_user(oldval, uaddr))
> > > > +		return -EFAULT;
> > > > +
> > > > +	/*
> > > > +	 * there are no ldteor/stteor instructions...
> > > > +	 */
> > > > +	for (i = 0; i < FUTEX_MAX_LOOPS; i++) {
> > > > +		newval = oldval ^ oparg;
> > > > +
> > > > +		ret = __lsui_cmpxchg32(uaddr, oldval, newval, &val);
> > >
> > > Since we have a FUTEX_MAX_LOOPS here, do we need it in cmpxchg32 as
> > > well?
> > >
> > > For eor, we need a loop irrespective of whether futex_pos or other_pos
> > > have changed. For cmpxchg, we need the loop only if other_pos has
> > > changed and return -EAGAIN if futex_pos has changed since the caller
> > > needs to update oldval and call again.
> > >
> > > So try to differentiate these cases, maybe only keep the loop outside
> > > cmpxchg32 (I haven't put much though into it).
> >
> > I think we can remove loops on __lsui_cmpxchg32() and return -EAGAIN
> > when other_pos is different. the __lsui_cmpxchg32() will be called
> > "futex_cmpxchg_value_locked()" and as I said, this always checks
> > whether curval & oldval when it successed.
>
> Yes, I think for the futex_cmpxchg_value_locked(), the bounded loop
> doesn't matter since the core would invoke it back on -EAGAIN. It's nice
> not to fail if the actual futex did not change but in practice it
> doesn't make any difference and I'd rather keep the code simple.
>
> > But in "eor" when it receive "-EAGAIN" from __lsui_cmxchg32()
> > we can simply continue the loop.
>
> Yes, for eor we need the bounded loop. Only return -EAGAIN to the user
> if we finished the loop and either __lsui_cmpxchg32() returned -EAGAIN
> or the updated on futex_pos failed.
>
> Thanks.
>

Thanks for confirmation, I've changed them as we discussed.
But let me sent it on v7.0-rc4 based with v17.
(Unfortunately, I sent v16 yesterday with missing of those review in
this thread).

--
Sincerely,
Yeoreum Yun