Re: [PATCH v12 03/21] futex: Move futex_queue() into futex_wait_setup()

["André Almeida" <andrealmeid@igalia.com>]

Em 16/04/2025 13:29, Sebastian Andrzej Siewior escreveu:
> From: Peter Zijlstra <peterz@infradead.org>
> 
> futex_wait_setup() has a weird calling convention in order to return
> hb to use as an argument to futex_queue().
> 
> Mostly such that requeue can have an extra test in between.
> 
> Reorder code a little to get rid of this and keep the hb usage inside
> futex_wait_setup().
> 
> [bigeasy: fixes]
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

[...]

> diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c
> index 25877d4f2f8f3..6cf10701294b4 100644
> --- a/kernel/futex/waitwake.c
> +++ b/kernel/futex/waitwake.c
> @@ -339,18 +339,8 @@ static long futex_wait_restart(struct restart_block *restart);
>    * @q:		the futex_q to queue up on
>    * @timeout:	the prepared hrtimer_sleeper, or null for no timeout
>    */
> -void futex_wait_queue(struct futex_hash_bucket *hb, struct futex_q *q,
> -			    struct hrtimer_sleeper *timeout)
> +void futex_do_wait(struct futex_q *q, struct hrtimer_sleeper *timeout)

Update the name in the kernel doc comment as well. Also drop from the 
comment the part that says "futex_queue() and ..."

>   {
> -	/*
> -	 * The task state is guaranteed to be set before another task can
> -	 * wake it. set_current_state() is implemented using smp_store_mb() and
> -	 * futex_queue() calls spin_unlock() upon completion, both serializing
> -	 * access to the hash list and forcing another memory barrier.
> -	 */
> -	set_current_state(TASK_INTERRUPTIBLE|TASK_FREEZABLE);
> -	futex_queue(q, hb, current);
> -
>   	/* Arm the timer */
>   	if (timeout)
>   		hrtimer_sleeper_start_expires(timeout, HRTIMER_MODE_ABS);
> @@ -578,7 +568,8 @@ int futex_wait_multiple(struct futex_vector *vs, unsigned int count,
>    * @val:	the expected value
>    * @flags:	futex flags (FLAGS_SHARED, etc.)
>    * @q:		the associated futex_q
> - * @hb:		storage for hash_bucket pointer to be returned to caller
> + * @key2:	the second futex_key if used for requeue PI
> + * task:	Task queueing this futex
>    *
>    * Setup the futex_q and locate the hash_bucket.  Get the futex value and
>    * compare it with the expected value.  Handle atomic faults internally.
> @@ -589,8 +580,10 @@ int futex_wait_multiple(struct futex_vector *vs, unsigned int count,
>    *  - <1 - -EFAULT or -EWOULDBLOCK (uaddr does not contain val) and hb is unlocked
>    */
>   int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
> -		     struct futex_q *q, struct futex_hash_bucket **hb)
> +		     struct futex_q *q, union futex_key *key2,
> +		     struct task_struct *task)
>   {
> +	struct futex_hash_bucket *hb;
>   	u32 uval;
>   	int ret;
>   
> @@ -618,12 +611,12 @@ int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
>   		return ret;
>   
>   retry_private:
> -	*hb = futex_q_lock(q);
> +	hb = futex_q_lock(q);
>   
>   	ret = futex_get_value_locked(&uval, uaddr);
>   
>   	if (ret) {
> -		futex_q_unlock(*hb);
> +		futex_q_unlock(hb);
>   
>   		ret = get_user(uval, uaddr);
>   		if (ret)
> @@ -636,10 +629,25 @@ int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
>   	}
>   
>   	if (uval != val) {
> -		futex_q_unlock(*hb);
> -		ret = -EWOULDBLOCK;
> +		futex_q_unlock(hb);
> +		return -EWOULDBLOCK;
>   	}
>   
> +	if (key2 && futex_match(&q->key, key2)) {
> +		futex_q_unlock(hb);
> +		return -EINVAL;

Please add this new ret value in the kernel doc too.