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

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

[Uros Bizjak]

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.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: "André Almeida" <andrealmeid@igalia.com>
---
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;
+	if (!atomic64_try_cmpxchg_relaxed(&inode->i_sequence, &old, new))
+		return old;
+	return new;
 }
 
 /**
-- 
2.46.2

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

[André Almeida]

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.

> 
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: "André Almeida" <andrealmeid@igalia.com>
> ---
> 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.

> +	if (!atomic64_try_cmpxchg_relaxed(&inode->i_sequence, &old, new))
> +		return old;
> +	return new;
>   }
>   
>   /**

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

[Thomas Gleixner]

On Wed, Oct 09 2024 at 16:43, André Almeida wrote:
> 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.

We are not expecting, trying, happy.. That's non-technical babbling.

See Documentation/process/* for futher explanation.

I agree with you that the change log should be more informative about
the why instead of listing the what, but not for the price of a
non-technical 'we' novel.

Thanks,

        tglx

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@igalia.com> 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@gmail.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Darren Hart <dvhart@infradead.org>
> > Cc: Davidlohr Bueso <dave@stgolabs.net>
> > Cc: "André Almeida" <andrealmeid@igalia.com>
> > ---
> > 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.