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.
--
Catalin
