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

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