Re: [PATCH v8 08/15] futex: Prepare for reference counting of the process private hash end of operation.

[Peter Zijlstra]

On Mon, Feb 03, 2025 at 02:59:28PM +0100, Sebastian Andrzej Siewior wrote:

> @@ -555,11 +558,12 @@ struct futex_hash_bucket *futex_q_lock(struct futex_q *q)
>  	return hb;
>  }
>  
> -void futex_q_unlock(struct futex_hash_bucket *hb)
> +void futex_q_unlock_put(struct futex_hash_bucket *hb)
>  	__releases(&hb->lock)
>  {
>  	futex_hb_waiters_dec(hb);
>  	spin_unlock(&hb->lock);
> +	futex_hash_put(hb);
>  }

Here you don't

> @@ -288,23 +289,29 @@ extern void __futex_unqueue(struct futex_q *q);
>  extern void __futex_queue(struct futex_q *q, struct futex_hash_bucket *hb);
>  extern int futex_unqueue(struct futex_q *q);
>  
> +static inline void futex_hb_unlock_put(struct futex_hash_bucket *hb)
> +{
> +	spin_unlock(&hb->lock);
> +	futex_hash_put(hb);
> +}
> +
>  /**
> - * futex_queue() - Enqueue the futex_q on the futex_hash_bucket
> + * futex_queue_put() - Enqueue the futex_q on the futex_hash_bucket
>   * @q:	The futex_q to enqueue
>   * @hb:	The destination hash bucket
>   *
> - * The hb->lock must be held by the caller, and is released here. A call to
> - * futex_queue() is typically paired with exactly one call to futex_unqueue().  The
> - * exceptions involve the PI related operations, which may use futex_unqueue_pi()
> - * or nothing if the unqueue is done as part of the wake process and the unqueue
> - * state is implicit in the state of woken task (see futex_wait_requeue_pi() for
> - * an example).
> + * The hb->lock must be held by the caller, and is released here and the reference
> + * on the hb is dropped. A call to futex_queue_put() is typically paired with
> + * exactly one call to futex_unqueue(). The exceptions involve the PI related
> + * operations, which may use futex_unqueue_pi() or nothing if the unqueue is
> + * done as part of the wake process and the unqueue state is implicit in the
> + * state of woken task (see futex_wait_requeue_pi() for an example).
>   */
> -static inline void futex_queue(struct futex_q *q, struct futex_hash_bucket *hb)
> +static inline void futex_queue_put(struct futex_q *q, struct futex_hash_bucket *hb)
>  	__releases(&hb->lock)
>  {
>  	__futex_queue(q, hb);
> -	spin_unlock(&hb->lock);
> +	futex_hb_unlock_put(hb);
>  }

And here you do.


> @@ -380,11 +387,13 @@ double_lock_hb(struct futex_hash_bucket *hb1, struct futex_hash_bucket *hb2)
>  }
>  
>  static inline void
> -double_unlock_hb(struct futex_hash_bucket *hb1, struct futex_hash_bucket *hb2)
> +double_unlock_hb_put(struct futex_hash_bucket *hb1, struct futex_hash_bucket *hb2)
>  {
>  	spin_unlock(&hb1->lock);
>  	if (hb1 != hb2)
>  		spin_unlock(&hb2->lock);
> +	futex_hash_put(hb1);
> +	futex_hash_put(hb2);
>  }
>  

This seems horribly inconsistent and makes my head hurt. Where are the
matching gets for double_lock_hb() ?