Re: [PATCH v2] futex: Rewrite get_inode_sequence_number() to make code simpler

[Uros Bizjak]

On Wed, Oct 9, 2024 at 9:44 PM André Almeida <andrealmeid@xxxxxxxxxx> wrote:
>
> Hi Uros,
>
> Em 04/10/2024 05:52, Uros Bizjak escreveu:
> > Rewrite get_inode_sequence_number() to make code simpler:
> >
> > a) Rewrite FOR loop to a DO-WHILE loop with returns moved
> > out of the loop.
> >
> > b) Use atomic64_inc_return() instead of atomic64_add_return().
> >
> > c) Use !atomic64_try_cmpxchg_relaxed(*ptr, &old, new) instead of
> > atomic64_cmpxchg_relaxed (*ptr, old, new) != old.  x86 CMPXCHG
> > instruction returns success in ZF flag, so this change also saves
> > a compare instruction after CMPXCHG.
>
> Remember, it's easy to see in the diff that you replace the function,
> but might be not so clear why you did so. I think it would be better to
> understand if you write like:
>
> We are trying to set a value for the i_sequence, that we expect that is
> zero, but if we fail to do so, we are happy to use the current non-zero
> i_sequence value that we found. Instead of using
> atomic64_cmpxchg_relaxed(), use atomic64_try_cmpxchg_relaxed() which
> provides a better semantic for this situation.

I will abandon the rewrite part and for the core changes post a
two-part patch series with two almost mechanical one liner patches
that I already have a widely accepted changelog template for.

Rewriting the loop form is mostly cosmetic, and since it doesn't have
an effect on code generation, I'm not that much interested in it. I'll
leave this part to eventual future patch submitter.

>
> >
> > Signed-off-by: Uros Bizjak <ubizjak@xxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Darren Hart <dvhart@xxxxxxxxxxxxx>
> > Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> > Cc: "André Almeida" <andrealmeid@xxxxxxxxxx>
> > ---
> > v2: Explicitly initialize "old" to zero before the call to
> > atomic64_try_cmpxchg_relaxed(). Rewrite commit message to
> > state the motivation for the patch.
> > ---
> >   kernel/futex/core.c | 18 ++++++++----------
> >   1 file changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/kernel/futex/core.c b/kernel/futex/core.c
> > index 136768ae2637..ac650f7ed56c 100644
> > --- a/kernel/futex/core.c
> > +++ b/kernel/futex/core.c
> > @@ -173,23 +173,21 @@ futex_setup_timer(ktime_t *time, struct hrtimer_sleeper *timeout,
> >   static u64 get_inode_sequence_number(struct inode *inode)
> >   {
> >       static atomic64_t i_seq;
> > -     u64 old;
> > +     u64 old, new;
> >
> >       /* Does the inode already have a sequence number? */
> >       old = atomic64_read(&inode->i_sequence);
> >       if (likely(old))
> >               return old;
> >
> > -     for (;;) {
> > -             u64 new = atomic64_add_return(1, &i_seq);
> > -             if (WARN_ON_ONCE(!new))
> > -                     continue;
> > +     do {
> > +             new = atomic64_inc_return(&i_seq);
> > +     } while (WARN_ON_ONCE(!new));
> >
> > -             old = atomic64_cmpxchg_relaxed(&inode->i_sequence, 0, new);
> > -             if (old)
> > -                     return old;
> > -             return new;
> > -     }
> > +     old = 0;
>
> Please initialize it in the variable declaration.

This is not possible, since we have an assignment from atomic64_read()
inbetween.

Thanks,
Uros.