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

Thomas Gleixner
2024-12-10T18:45:58Z

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