Re: [PATCH v4 05/11] futex: Track the futex hash bucket.

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Dec 03 2024 at 17:42, Sebastian Andrzej Siewior wrote:
> Add futex_hash_get/put() to keep the assigned hash_bucket around while a
> futex operation is performed. Have RCU lifetime guarantee for
> futex_hash_bucket_private.
>
> This is should have the right amount of gets/ puts so that the private

This is should have? This either has or not :)

>  struct futex_hash_bucket *futex_hash(union futex_key *key)
>  {
> -	struct futex_hash_bucket *fhb;
> +	struct futex_hash_bucket_private *hb_p = NULL;
>  	u32 hash;
>  
> -	fhb = current->mm->futex_hash_bucket;
> -	if (fhb && futex_key_is_private(key)) {
> -		u32 hash_mask = current->mm->futex_hash_mask;
> +	if (futex_key_is_private(key)) {
> +		guard(rcu)();
> +
> +		do {
> +			hb_p = rcu_dereference(current->mm->futex_hash_bucket);
> +		} while (hb_p && !rcuref_get(&hb_p->users));

This loop really wants an explanation about the potential loop
duration.

> +void futex_hash_put(struct futex_hash_bucket *hb)
> +{
> +	struct futex_hash_bucket_private *hb_p;
> +
> +	if (hb->hb_slot == 0)
> +		return;
> +	hb_p = container_of(hb, struct futex_hash_bucket_private,
> +			    queues[hb->hb_slot - 1]);

Duh. This off by one abuse of hb_slot is really counter intuitive. It
took me a while to wrap my head around it.

The structure has a 4 byte hole, so adding a private flag or such is
feasible without going over a cache line, unless lockdep or rt is
enabled, but in that case it expands into a second cache line anyway.

> +	futex_hash_priv_put(hb_p);
> +}
> +
> +void futex_hash_get(struct futex_hash_bucket *hb)
> +{
> +	struct futex_hash_bucket_private *hb_p;
> +
> +	if (hb->hb_slot == 0)
> +		return;
> +
> +	hb_p = container_of(hb, struct futex_hash_bucket_private,
> +			    queues[hb->hb_slot - 1]);
> +	/* The ref needs to be owned by the caller so this can't fail */

reference please. This is not twatter. But see below.

> +	WARN_ON_ONCE(!rcuref_get(&hb_p->users));
> +}
>  
>  /**
>   * futex_setup_timer - set up the sleeping hrtimer.
> @@ -599,7 +642,10 @@ int futex_unqueue(struct futex_q *q)
>  	 */
>  	lock_ptr = READ_ONCE(q->lock_ptr);
>  	if (lock_ptr != NULL) {
> +		struct futex_hash_bucket *hb;
> +
>  		spin_lock(lock_ptr);
> +		hb = futex_hb_from_futex_q(q);
>  		/*
>  		 * q->lock_ptr can change between reading it and
>  		 * spin_lock(), causing us to take the wrong lock.  This
> @@ -622,6 +668,7 @@ int futex_unqueue(struct futex_q *q)
>  		BUG_ON(q->pi_state);
>  
>  		spin_unlock(lock_ptr);
> +		futex_hash_put(hb);

This is invoked from futex_wait_multiple() which means you are
are holding the reference count accross schedule(),

I'm not convinced that this is the right thing to do. Let me look at
your actual resize implementation...

>  		futex_q_unlock(hb);
> +		futex_hash_put(hb);

This pattern is there in a gazillion instances. Can't we have a single
function doing all of it?

Thanks,

        tglx