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
