Re: [PATCH v5 06/14] futex: Add helper which include the put of a hb after end of operation.

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Dec 16 2024 at 00:00, Sebastian Andrzej Siewior wrote:
> With the planned schema of resize of hb, a reference count will be
> obtained during futex_hash() and will be dropped after the hb is
> unlocked. Once the reference is dropped, the hb must not be used because
> it will disappear after a resize.
> To prepare the integration, rename
> - futex_hb_unlock() to futex_hb_unlock_put()
> - futex_queue() to futex_queue_put()
> - futex_q_unlock() to futex_q_unlock_put()
> - double_unlock_hb() to double_unlock_hb_put()
>
> which is additionally includes futex_hb_unlock_put(), an empty stub.
> Introduce futex_hb_unlock_put() which is the unlock plus the reference
> drop. Move futex_hb_waiters_dec() before the reference drop, if needed
> before the unlock.
> Update comments referring to the functions accordingly.

If I didn't know what you are talking about, it would have taken me a
while to decode the word salad above. It starts with the usage of 'hb',
an acronym which might be understandable for people familiar with the
futex code, but otherwise it's an arbitrary reference to nothing.

Assuming that 'hb' stands for hashbucket, the usage here is wrong:

      With the planned schema of resize of hb...

This is about resizing the hash not the hashbucket, right?

So something like this might be more to the point:

   futex: Prepare for reference counting of the process private hash

   To support runtime resizing of the process private hash, it's
   required to add a reference count to the hash structure. The
   reference count ensures that the hash cannot be resized or freed
   while a task is operating on it.

   The reference count will be obtained within futex_hash() and dropped
   once the hash bucket is unlocked and not longer required for the
   particular operation (queue, unqueue, wakeup etc.).

   This is achieved by:

    - appending _put() to existing functions so it's clear that they
      also put the hash reference and fixing up the usage sites

    - providing new helpers, which combine common operations (unlock,
      put), and using them at the appropriate places

    - providing new helper for standalone reference counting
      functionality and using them at places, where the unlock operation
      needs to be separate.
   
Hmm?
> -void futex_q_unlock(struct futex_hash_bucket *hb)
> +void futex_q_unlock_put(struct futex_hash_bucket *hb)
>  	__releases(&hb->lock)
>  {
>  	spin_unlock(&hb->lock);
>  	futex_hb_waiters_dec(hb);
> +	futex_hash_put(hb);

You missed a place to move the waiter_dec() before the unlock. Actually
this move should be in a separate preparatory patch, which does only
that. It also needs a proper change log which explains why this done,
why it is equivalent and not introducing a change in terms of ordering
rules. This:

> Move futex_hb_waiters_dec() before the reference drop, if needed
> before the unlock.

does not really give any clue at all.

>  		if (unlikely(ret)) {
> -			double_unlock_hb(hb1, hb2);
>  			futex_hb_waiters_dec(hb2);
> +			double_unlock_hb_put(hb1, hb2);

And having this move before the _put() change makes the latter a purely
mechanical change which let's the reader/reviewer focus on the reference
count rules and not be distracted by the waiter count changes.
  
> -	/* futex_queue and wait for wakeup, timeout, or a signal. */
> +	/* futex_queue_put and wait for wakeup, timeout, or a signal. */

When you fix up these comments, can you please use the fn() notation?

>  	futex_wait_queue(hb, &q, to);
>  
>  	/* If we were woken (and unqueued), we succeeded, whatever. */

Thanks,

        tglx